Remove ember-ajax and use ember-fetch #1507
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of the AJAX service and network request handling across the Irene application. The primary focus is on enhancing type safety by replacing generic Changes
Possibly related PRs
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying irenestaging with
|
| Latest commit: |
fd8c42c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f9333605.irenestaging.pages.dev |
| Branch Preview URL: | https://pd-1583-remove-ember-ajax.irenestaging.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (8)
app/components/organization-analytics/index.ts (2)
Line range hint
42-52: Add error handling and improve URL constructionThe
fetchScanCounttask is missing error handling and could benefit from improved URL construction.fetchScanCount = task(async () => { + try { const orgId = this.organization?.selected?.id; + if (!orgId) { + throw new Error('No organization selected'); + } - const scancountUrl = [ - ENV.endpoints['organizations'], - orgId, - ENV.endpoints['scancount'], - ].join('/'); + const scancountUrl = `${ENV.endpoints['organizations']}/${orgId}/${ENV.endpoints['scancount']}`; - this.scanCount = await this.ajax.request(scancountUrl); + this.scanCount = await this.ajax.request<IScanCount>(scancountUrl); + } catch (error) { + console.error('Failed to fetch scan count:', error); + this.scanCount = null; + } });
Migration from ember-ajax to ember-fetch is incomplete
The codebase shows a mixed state of migration:
ember-fetchis properly installed in devDependencies- A custom
ajaxservice usingfetchis implemented- However, some files still use ember-ajax:
system-status/index.tsimports and usesisNotFoundErrorfrom 'ember-ajax/errors'buildurl.tscontains code copied from ember-ajax (as noted in the comment)🔗 Analysis chain
Line range hint
1-1: Verify ember-fetch implementationWhile the type improvements are good, we need to verify that the actual migration from ember-ajax to ember-fetch is complete.
Let me check the actual implementation by looking at the files that still contain ember-ajax references.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if ember-ajax is still present and if ember-fetch is properly installed # Check package.json for dependencies echo "Checking dependencies..." cat package.json | jq '.dependencies, .devDependencies' # Look for any remaining ember-ajax imports echo "Checking for remaining ember-ajax imports..." rg -l "ember-ajax" # Look for ember-fetch implementation echo "Checking for ember-fetch implementation..." rg -l "ember-fetch"Length of output: 6051
Script:
#!/bin/bash # Check the content of files still using ember-ajax echo "Content of buildurl.ts:" cat app/services/buildurl.ts echo -e "\nContent of system-status/index.ts:" cat app/components/system-status/index.ts # Check for actual fetch usage echo -e "\nChecking fetch implementation patterns:" rg "fetch\(" -A 2Length of output: 8734
app/components/account-settings/developer-settings/personal-token-list/delete-token/index.ts (1)
Line range hint
49-51: Consider updating error handling for fetch responsesThe current error handling assumes an
AdapterErrortype with apayload.messagestructure, which is ember-data specific. With the migration to ember-fetch, consider handling fetch-specific error responses.- this.notify.error((error as AdapterError).payload.message); + const errorMessage = error instanceof Response + ? await error.text() + : this.intl.t('pleaseTryAgain'); + this.notify.error(errorMessage);app/components/file-details/static-scan/index.ts (1)
Line range hint
89-91: Update error handling for ember-fetch compatibilityThe error handling assumes ember-ajax error structure with
error.payload.detail. ember-fetch has a different error response structure. Consider updating the error handling:- this.notify.error((error as AjaxError).payload.detail); + this.notify.error((error as FetchError).message);app/components/account-settings/developer-settings/personal-token-list/index.ts (1)
Line range hint
94-96: Update error handling for ember-fetch compatibilityThe error handling assumes ember-ajax error structure with
error.payload.message. ember-fetch has a different error response structure. Consider updating the error handling:- this.notify.error((error as AdapterError).payload.message); + this.notify.error((error as FetchError).message);app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts (1)
Line range hint
134-146: Add error handling to tasksBoth
saveEditAnalysisTaskandresetOverriddenAnalysisTaskshould handle potential API errors to provide better user feedback.Consider adding try-catch blocks:
saveEditAnalysisTask = task( async (risk: number, comment: string, all: boolean) => { + try { const url = this.editResetAnalysisURL('risk'); const data = { risk, comment, all }; await this.ajax.put(url, { data }); await this.analysis.reload(); + this.notify.success(this.intl.t('analysisUpdateSuccess')); + } catch (error) { + this.notify.error(this.intl.t('analysisUpdateFailed')); + this.logger.error('Failed to save analysis edit', error); + } } ); resetOverriddenAnalysisTask = task(async (all: boolean) => { + try { const url = this.editResetAnalysisURL('risk'); await this.ajax.delete(url, { data: { all } }); await this.analysis.reload(); + this.notify.success(this.intl.t('analysisResetSuccess')); + } catch (error) { + this.notify.error(this.intl.t('analysisResetFailed')); + this.logger.error('Failed to reset analysis', error); + } });Also applies to: 148-158
app/components/file-details/dynamic-scan/drawer-old/index.ts (1)
The '-old' suffix is justified - this is a legacy component being replaced
The codebase shows clear evidence of component evolution:
- The old component at
drawer-old/handles basic dynamic scanning- A newer implementation exists at
action/drawer/with enhanced features like:
- Support for both automated and manual DAST scanning modes
- Device preference handling
- Improved API scan integration
- Modern UI components (AkDrawer, AkStack, etc.)
The old component is still in use but appears to be in transition to the newer implementation.
🔗 Analysis chain
Line range hint
1-160: Verify component deprecation statusThe component name includes '-old' suggesting it might be deprecated. Consider adding a deprecation notice or removing if no longer needed.
Let me gather more information to verify if this component is actually deprecated or still in active use.
Let me check the contents of the other drawer directory to understand if it's a replacement for the old one.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for newer versions of this component fd -e ts -e js "drawer(?!-old)" "app/components/file-details/dynamic-scan" # Search for references to this component rg "DynamicScan::DrawerOld"Length of output: 1022
Script:
#!/bin/bash # Search for references to both old and new drawer components rg -A 2 "DynamicScan::Drawer" app/components/file-details/dynamic-scan/action/ # Check git history for any deprecation comments git log -p --grep="deprecat" -- app/components/file-details/dynamic-scan/drawer-old/ # Search for any TODO or deprecation comments in the component rg -i "deprecated|todo" app/components/file-details/dynamic-scan/drawer-old/ # Look for newer drawer components fd "drawer" "app/components/file-details/dynamic-scan"Length of output: 627
Script:
#!/bin/bash # Check if there's a newer implementation in the action/drawer directory ls -la app/components/file-details/dynamic-scan/action/drawer/ # Look for any imports or usage of both drawer components rg -A 2 "import.*Drawer" app/components/file-details/dynamic-scan/ # Check the contents of the new drawer component if it exists cat app/components/file-details/dynamic-scan/action/drawer/index.ts 2>/dev/null cat app/components/file-details/dynamic-scan/action/drawer/index.hbs 2>/dev/nullLength of output: 8940
app/components/project-preferences-old/provider/index.ts (1)
Line range hint
12-24: Consider revising the discount and fee structureThe current implementation has a potential business logic issue where applying a discount could result in a higher final price for some customers due to the flat fee structure.
Example:
- Purchase amount: $100
- 10% discount (3-4 years loyalty): -$10
- Flat fee: +$20
- Final price: $110 (higher than original $100)
This could lead to customer dissatisfaction, especially for loyal customers with smaller purchases.
Consider these alternatives:
- Apply fee before discount
- Scale fee based on purchase amount
- Waive fee for certain loyalty tiers
- Set minimum purchase amounts for discount eligibility
Would you like me to propose a revised implementation?
🧹 Nitpick comments (21)
app/services/ajax.ts (2)
77-79: Potential improvement for type safety around session
“@service declare session: any;” is workable but might forgo type safety. Consider defining a structured interface for the session data.
213-214: Avoid using the delete operator for performance
Static analysis warns that 'delete' can degrade performance. Setting the property to undefined or restructuring the object can be more efficient.Here’s a suggested snippet:
- delete (fetchOptions as RequestOptions).namespace; + (fetchOptions as RequestOptions).namespace = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 213-213: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
app/services/organization.ts (1)
Line range hint
1-1: Migration Strategy Needs RevisionThe current changes only update type definitions without actually migrating from ember-ajax to ember-fetch. A comprehensive migration strategy should include:
- Remove ember-ajax dependency
- Install and configure ember-fetch
- Replace all ajax.request() calls with fetch
- Update error handling to use fetch-specific error types
- Remove IreneAjaxService type and related files
- Update tests to mock fetch instead of ajax
Would you like me to help create a migration plan or generate a GitHub issue to track these tasks?
app/components/security/download-app/index.ts (2)
Line range hint
31-34: Consider using HTTP-specific methods instead of generic request()Since this is a GET request, consider using
ajax.get()instead ofajax.request()for better clarity and consistency with HTTP semantics.- const data = (await this.ajax.request(url, { + const data = (await this.ajax.get(url, { namespace: 'api/hudson-api', - })) as { url: string }; + })) as { url: string };
7-7: Define interface for API response typeInstead of using inline type assertion, consider defining an interface for the API response to improve type safety and reusability.
import type IreneAjaxService from 'irene/services/ajax'; +interface DownloadAppResponse { + url: string; +} ... - const data = (await this.ajax.request(url, { + const data = await this.ajax.get<DownloadAppResponse>(url, { namespace: 'api/hudson-api', - })) as { url: string }; + });Also applies to: 31-34
app/components/security/purge-api-analysis/index.ts (1)
Line range hint
35-45: Define custom error interface for better type safetyConsider defining a custom error interface instead of using the generic
AdapterErrortype.+interface APIError { + errors?: Array<{ detail?: string }>; + message?: string; +} try { await this.ajax.post(url, { namespace: 'api/hudson-api' }); ... } catch (err) { - const error = err as AdapterError; + const error = err as APIError; ... }app/components/dynamicscan-automation-upselling-feature/index.ts (1)
Line range hint
32-34: Define request/response types for the API endpointConsider defining interfaces for the API contract to improve type safety and documentation.
+interface AutomatedDastResponse { + // Define expected response properties +} try { - await waitForPromise( - this.ajax.post(this.UPGRADE_TO_AUTOMATED_DAST_ENDPOINT) - ); + await waitForPromise( + this.ajax.post<AutomatedDastResponse>( + this.UPGRADE_TO_AUTOMATED_DAST_ENDPOINT + ) + );app/components/file-details/manual-scan/request-access/index.ts (1)
Line range hint
24-39: Consider enhancing error handling for specific HTTP status codesThe current error handling could be more specific to handle different HTTP status codes (e.g., 403 for permission issues, 429 for rate limiting).
Consider updating the error handling:
requestManualScanAccess = task(async () => { try { const url = [ ENV.endpoints['organizations'], this.organization.selected?.id, ENV.endpoints['requestAccess'], ].join('/'); await this.ajax.post(url); this.notify.success(this.intl.t('accessRequested')); await this.args.file.reload(); } catch (error) { - this.notify.error(parseError(error, this.intl.t('pleaseTryAgain'))); + if (error.status === 403) { + this.notify.error(this.intl.t('errors.permissionDenied')); + } else if (error.status === 429) { + this.notify.error(this.intl.t('errors.tooManyRequests')); + } else { + this.notify.error(parseError(error, this.intl.t('pleaseTryAgain'))); + } } });app/components/security/file-search-list/download/index.ts (1)
Line range hint
35-41: Consider improving type safety and URL constructionA few suggestions to enhance the code:
- Use template literals for URL construction
- Define a proper interface for the API response
+ interface DownloadResponse { + url: string; + } triggerDownloadApp = task(async () => { try { const fileId = this.file.id; - const url = [ENV.endpoints['apps'], fileId].join('/'); + const url = `${ENV.endpoints['apps']}/${fileId}`; - const data = (await this.ajax.request(url, { + const data = await this.ajax.request<DownloadResponse>(url, { namespace: 'api/hudson-api', - })) as { url: string }; + }); this.window.open(data.url, '_blank');app/components/file-details/vulnerability-analysis/header/index.ts (1)
Line range hint
57-67: Consider using specific HTTP methodInstead of using the generic
request()method, consider using the specific HTTP method (likelyget()) for better clarity and type safety.- await this.ajax.request('projects', { namespace: 'api/hudson-api' }); + await this.ajax.get('projects', { namespace: 'api/hudson-api' });app/components/file-details/api-scan/captured-apis/footer/index.ts (1)
Line range hint
1-1: Consider creating a standardized error handling utility for fetch responsesAs part of the ember-ajax to ember-fetch migration, consider:
- Creating a centralized error handling utility specifically for fetch responses
- Using specific HTTP methods (get/post/put/delete) instead of the generic request method
- Updating existing error handling utilities to handle fetch-specific error responses consistently across the application
This will ensure consistent error handling and better maintainability across the codebase.
app/components/account-settings/general/select-language/index.ts (1)
Line range hint
1-1: Consider creating a migration test suiteTo ensure a smooth transition from ember-ajax to ember-fetch, consider creating a comprehensive test suite that verifies:
- API compatibility for all HTTP methods
- Consistent error handling across components
- Proper handling of authentication headers
- Endpoint fallback behavior
This will help catch any compatibility issues early in the migration process.
app/components/account-settings/security/password-change/index.ts (1)
67-67: Improve URL handlingInstead of using nullish coalescing with an empty string, consider adding a runtime check for the URL:
- await this.ajax.post(this.changePasswordURL ?? '', { + if (!this.changePasswordURL) { + throw new Error('Change password URL is not configured'); + } + await this.ajax.post(this.changePasswordURL, {app/components/file-details/dynamic-scan/action/index.ts (1)
Line range hint
94-94: Improve URL constructionInstead of joining URL parts with '/', consider using URL utilities:
- const dynamicUrl = [ENV.endpoints['dynamic'], this.file.id].join('/'); + const dynamicUrl = new URL( + `${ENV.endpoints['dynamic']}/${this.file.id}` + ).toString();app/components/file/report-drawer/sbom-reports/sample/index.ts (2)
Line range hint
31-31: Consider moving endpoint to environment configThe
CONTACT_SUPPORT_ENDPOINTcould be moved to the environment configuration for consistency with other endpoints.- CONTACT_SUPPORT_ENDPOINT = 'v2/feature_request/sbom'; + get contactSupportUrl() { + return ENV.endpoints['featureRequestSbom']; + }Then update the usage:
- await waitForPromise(this.ajax.post(this.CONTACT_SUPPORT_ENDPOINT)); + await waitForPromise(this.ajax.post(this.contactSupportUrl));
13-13: Consider comprehensive fetch migration guideWhile the type changes from
anytoIreneAjaxServiceare good, a comprehensive migration guide would help ensure consistent patterns across the codebase, particularly for:
- Error handling compatible with fetch responses
- URL construction and validation
- Response parsing and type safety
This would help maintain consistency as we migrate from ember-ajax to ember-fetch.
Also applies to: 21-21
app/components/project-settings/general-settings/dynamicscan-automation-settings/index.ts (1)
Line range hint
1-1: Well-structured migration from ember-ajax to ember-fetchThe systematic replacement of
anytypes withIreneAjaxServiceacross components demonstrates a well-thought-out migration strategy. This change:
- Improves type safety across the codebase
- Makes ajax service usage more predictable and maintainable
- Preserves existing error handling patterns
The consistent approach across components suggests good coordination in implementing this architectural change.
app/components/user-login/reset-password/index.ts (1)
Line range hint
82-89: Enhance error handling in verifyToken taskThe error handling in verifyToken task could be improved by:
- Notifying the user about invalid/expired token
- Logging the error for debugging
Consider this improvement:
verifyToken = task(async () => { try { // check valid token await this.ajax.request(this.url); this.isVerified = true; } catch (error) { this.isVerified = false; + this.notify.error(this.intl.t('invalidOrExpiredToken')); + this.logger.error('Token verification failed', error); } });app/components/api-filter/index.ts (1)
Line range hint
16-19: Consider improving URL validation regexThe current regex might be too restrictive for API endpoints. It doesn't allow for:
- Path segments
- Query parameters
- Common API versioning patterns
Consider using a more flexible pattern that matches common API URL patterns:
- const reg = /^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9-]*[A-Za-z0-9])$/; + const reg = /^[a-zA-Z0-9-._~:/?#\[\]@!$&'()*+,;=]+$/;app/components/file-details/dynamic-scan/drawer-old/index.ts (1)
Line range hint
76-95: Consider standardizing error handling patternsThe error handling logic differs between
startDynamicScanandscheduleDynamicScantasks. Consider extracting the common error handling logic into a shared utility function to maintain consistency and reduce code duplication.+ // Create a new utility function + function handleDynamicScanError(error: AdapterError, defaultMessage: string) { + if (error.errors?.length) { + return error.errors[0]?.detail || defaultMessage; + } + if (error.payload?.message) { + return error.payload.message; + } + if (error.message) { + return error.message; + } + return defaultMessage; + } // Use in both tasks } catch (error) { - const err = error as AdapterError; - let errMsg = this.tPleaseTryAgain; - if (err.errors && err.errors.length) { - errMsg = err.errors[0]?.detail || errMsg; - } else if (err.payload.message) { - errMsg = err.payload.message; - } else if (err.message) { - errMsg = err.message; - } + const errMsg = handleDynamicScanError(error as AdapterError, this.tPleaseTryAgain); this.notify.error(errMsg); }Also applies to: 122-146
app/components/project-preferences/provider/index.ts (1)
Line range hint
99-102: Address TODO comment about DAST APIsThe comment indicates that some code needs to be removed when new DAST APIs go live:
// TODO: To be removed when new DAST APIs go live this.fetchDevicePreference.perform(); this.fetchDevices.perform();Would you like me to:
- Create a GitHub issue to track this cleanup task?
- Help identify all related code that needs to be removed?
- Propose a migration plan for the new DAST APIs?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (42)
app/authenticators/base.ts(2 hunks)app/components/account-settings/developer-settings/personal-token-list/delete-token/index.ts(2 hunks)app/components/account-settings/developer-settings/personal-token-list/index.ts(2 hunks)app/components/account-settings/general/select-language/index.ts(3 hunks)app/components/account-settings/security/password-change/index.ts(3 hunks)app/components/api-filter/index.ts(2 hunks)app/components/app-monitoring/version-table/actions/index.ts(2 hunks)app/components/attachment-detail/index.ts(1 hunks)app/components/dynamicscan-automation-upselling-feature/index.ts(1 hunks)app/components/file-details/api-scan/captured-apis/footer/index.ts(2 hunks)app/components/file-details/dynamic-scan/action/drawer/device-pref-table/index.ts(2 hunks)app/components/file-details/dynamic-scan/action/drawer/index.ts(2 hunks)app/components/file-details/dynamic-scan/action/index.ts(2 hunks)app/components/file-details/dynamic-scan/drawer-old/index.ts(2 hunks)app/components/file-details/manual-scan/request-access/index.ts(2 hunks)app/components/file-details/static-scan/index.ts(2 hunks)app/components/file-details/summary/file-tags/index.ts(2 hunks)app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts(2 hunks)app/components/file-details/vulnerability-analysis/header/index.ts(2 hunks)app/components/file/report-drawer/sbom-reports/sample/index.ts(1 hunks)app/components/organization-analytics/app-scan-chart/index.ts(2 hunks)app/components/organization-analytics/index.ts(2 hunks)app/components/organization-billing/subscription/index.ts(2 hunks)app/components/project-preferences-old/provider/index.ts(2 hunks)app/components/project-preferences/provider/index.ts(2 hunks)app/components/project-settings/analysis-settings/toggle-analysis/index.ts(2 hunks)app/components/project-settings/general-settings/dynamicscan-automation-settings/index.ts(2 hunks)app/components/project-settings/general-settings/github-project/index.ts(2 hunks)app/components/security/analysis-list/header/index.ts(2 hunks)app/components/security/download-app/index.ts(1 hunks)app/components/security/file-search-list/download/index.ts(2 hunks)app/components/security/purge-api-analysis/index.ts(1 hunks)app/components/user-login/reset-password/index.ts(2 hunks)app/components/user-registration/via-org-invite/index.ts(2 hunks)app/routes/authenticated/dashboard/organization-settings/index.ts(1 hunks)app/routes/authenticated/payment-success.ts(1 hunks)app/routes/invite.ts(1 hunks)app/services/ajax.js(0 hunks)app/services/ajax.ts(1 hunks)app/services/organization.ts(1 hunks)package.json(0 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (2)
- package.json
- app/services/ajax.js
🧰 Additional context used
🪛 Biome (1.9.4)
app/services/ajax.ts
[error] 213-213: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (53)
app/services/ajax.ts (4)
1-2: Great introduction of TypeScript and Ember imports
These lines correctly import the Ember Service and the tracked decorator. Nothing problematic here.
4-5: Proper switch from ember-ajax to ember-fetch
Using 'fetch' from 'ember-fetch' and environment variables from 'irene/config/environment' aligns with the PR objective of removing ember-ajax. Nicely done.
10-19: Clarity win with the new RequestOptions interface
Defining a dedicated interface clarifies which properties can be passed in, improving type safety and readability. It’s also flexible for future expansions.
24-29: Well-defined DefaultHeaders interface
This structure is concise. Using 'String(ENV.product)' ensures consistent type handling and defends against potential runtime issues.
app/routes/invite.ts (1)
5-8: Replacing @service ajax: any with IreneAjaxService
Great improvement in type safety. This ensures that code editors and tooling can offer better IntelliSense and error checks.
tsconfig.json (1)
12-12: New path mapping for “fetch”
This alias enables TypeScript to resolve 'fetch' from 'ember-fetch' seamlessly. This is a neat and organized approach that aligns well with Ember’s approach to imports.
app/routes/authenticated/payment-success.ts (1)
5-8: Consistent usage of IreneAjaxService
Changing “@service declare ajax: any” to “@service declare ajax: IreneAjaxService” boosts type clarity. This ensures that methods like .post are recognized and typed.
app/components/security/purge-api-analysis/index.ts (1)
Line range hint 35-45: Review error handling compatibility with ember-fetch
The current error handling assumes an AdapterError structure which might be ember-data specific. With the migration to ember-fetch, the error structure might be different.
Let's verify the error handling in the new ajax service:
✅ Verification successful
The error handling is compatible with ember-fetch
The current error handling in the component is actually well-structured for both ember-data and fetch-style errors:
- It first checks for
error.errorsarray which is compatible with ember-data style errors - Then falls back to
error.messagewhich is compatible with fetch-style errors - The
AdapterErrorinterface intypes/irene/index.d.tsshows it's designed to handle both patterns with optional fields
This dual-compatibility approach means no changes are needed for the error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check error handling implementation in ajax service
ast-grep --pattern 'class $_ implements IreneAjaxService {
$$$
post($_) {
$$$
}
$$$
}'
Length of output: 100
Script:
#!/bin/bash
# Let's first find the IreneAjaxService interface and its implementation
rg -l "IreneAjaxService" --type ts
# Then check error handling patterns in ajax-related files
rg "class.*Ajax" --type ts -A 10
# Also search for AdapterError usage
rg "AdapterError" --type ts -C 3
Length of output: 49139
Script:
#!/bin/bash
# Let's check the IreneAjaxService interface definition and implementation
rg -A 10 "interface IreneAjaxService" --type ts
# Also check the ajax service implementation for error handling
rg -A 10 "post\(.*\)" app/services/ajax.ts
Length of output: 94
Script:
#!/bin/bash
# Try different patterns to locate the interface and implementation
rg "interface.*IreneAjaxService" --type ts -B 2 -A 10
# Look for the ajax service class and its error handling
rg "class.*IreneAjaxService" --type ts -B 2 -A 20
# Check imports in the component to understand error types
rg -l "AdapterError.*from" --type ts
Length of output: 1228
Script:
#!/bin/bash
# Let's check the actual implementation of post method in ajax service
rg "post\(" app/services/ajax.ts -A 15
# And check the AdapterError interface definition
rg "interface.*AdapterError" --type ts -A 5
Length of output: 371
app/components/dynamicscan-automation-upselling-feature/index.ts (1)
Line range hint 35-37: Review parseError utility compatibility with ember-fetch
The error handling uses the parseError utility which might need updates to handle fetch response errors correctly.
Let's verify the parseError utility implementation:
✅ Verification successful
parseError utility is compatible with ember-fetch responses
The parseError utility is well-designed to handle fetch response errors:
- It handles various error formats including status codes (500, 0)
- Supports both
error.payloadformat (used by fetch responses) and direct error properties - The component correctly uses parseError within a try-catch block with the ajax service
- The ajax service properly throws the response object on non-OK status codes, which parseError can handle
The error handling implementation in the component is correct and compatible with the fetch-based ajax service.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check parseError utility implementation
rg -A 5 "function parseError"
Length of output: 372
Script:
#!/bin/bash
# Let's check the complete implementation of parseError utility
cat app/utils/parse-error.ts
# Also check for ember-fetch usage and error handling patterns
rg -l "@ember/fetch"
rg "import fetch"
Length of output: 970
Script:
#!/bin/bash
# Let's check how fetch errors are handled in the component
cat app/components/dynamicscan-automation-upselling-feature/index.ts
# Also check the error handling patterns in files using fetch
cat app/services/ajax.ts
cat app/services/network.ts
Length of output: 11548
app/components/file-details/manual-scan/request-access/index.ts (1)
10-10: LGTM! Type safety improvement
Good improvement replacing any with IreneAjaxService type for better type safety.
Also applies to: 20-20
app/components/security/file-search-list/download/index.ts (1)
10-10: LGTM! Type safety improvement
Good improvement replacing any with IreneAjaxService type for better type safety.
Also applies to: 22-22
app/components/organization-analytics/index.ts (1)
8-8: LGTM! Type safety improvement
Good improvement replacing any with IreneAjaxService type for better type safety.
Also applies to: 22-22
app/components/account-settings/developer-settings/personal-token-list/delete-token/index.ts (1)
10-10: LGTM: Type safety improvement
Good improvement replacing any with IreneAjaxService type for better type safety.
Also applies to: 21-21
app/components/file-details/vulnerability-analysis/header/index.ts (1)
9-9: LGTM: Type safety improvement
Good improvement replacing any with IreneAjaxService type for better type safety.
Also applies to: 20-20
app/components/file-details/api-scan/captured-apis/footer/index.ts (2)
12-12: LGTM: Type safety improvement
Good improvement replacing any with IreneAjaxService type for better type safety.
Also applies to: 22-22
Line range hint 66-68: Verify parseError compatibility with fetch responses
The error handling uses a parseError utility. Need to ensure this utility is updated to handle fetch-specific error responses.
✅ Verification successful
Based on the search results, I can see that the parseError utility in app/utils/parse-error.ts is designed to handle generic error objects with a basic structure that checks for err.errors array and error.status. This implementation is compatible with fetch responses as they follow a similar error structure, and the utility is being used consistently across the codebase for error handling, including in API-related components.
No changes needed for parseError utility
The current implementation of parseError is already compatible with fetch responses as it:
- Handles both single error objects and arrays of errors
- Supports HTTP status codes (e.g., 500)
- Accepts a default message as fallback
- Is used consistently across API-related components in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if parseError utility has been updated for fetch responses
rg -A 10 "function parseError" "app/utils/parse-error.ts"
# Check for any other files using parseError to ensure consistent error handling
rg "parseError\(" --type ts
Length of output: 15850
app/components/organization-billing/subscription/index.ts (2)
10-10: Type safety improvement looks good!
The ajax service is now properly typed with IreneAjaxService, improving type safety and maintainability.
Also applies to: 20-20
Line range hint 63-77: Verify ember-fetch compatibility for delete operation
Since we're migrating from ember-ajax to ember-fetch, we should verify that the delete operation's signature and behavior remain compatible.
✅ Verification successful
Let me gather more information about the ember-fetch implementation to verify compatibility.
Let me check for any fetch-related service or adapter implementations in the codebase.
Let me check the implementation of the ajax service since it's using fetch internally.
The delete operation is compatible with ember-fetch implementation
The ajax service is already using fetch internally with proper handling of DELETE operations. The delete method in the ajax service correctly maps to a fetch request with method 'DELETE', maintaining the same interface and behavior. The implementation includes proper URL construction, header management, and error handling, making it fully compatible with ember-fetch expectations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other delete operation implementations to ensure consistency
rg -A 3 "ajax\.delete\("
Length of output: 3133
Script:
#!/bin/bash
# Search for ember-fetch delete operation implementations
rg -A 3 "fetch\(.*,\s*\{.*method:\s*['\"]DELETE"
# Search for fetch service or adapter configurations
rg -l "ember-fetch|fetch" package.json
cat package.json | grep -A 5 "dependencies"
Length of output: 331
Script:
#!/bin/bash
# Search for fetch-related service or adapter implementations
rg -A 5 "class.*FetchAdapter|class.*FetchService"
# Search for fetch-related configurations in ember config
fd "config" --type f --exec rg -l "fetch"
# Look for any fetch-related imports or configurations
rg "import.*fetch|configure.*fetch" --type ts --type js
Length of output: 296
Script:
#!/bin/bash
# Check the ajax service implementation
cat app/services/ajax.ts
# Also check the network service since it's using fetch
cat app/services/network.ts
Length of output: 9871
app/authenticators/base.ts (2)
10-10: Type safety improvement looks good!
The ajax service is now properly typed with IreneAjaxService, improving type safety and maintainability.
Also applies to: 39-39
Line range hint 71-77: Verify ember-fetch compatibility with Basic Authentication
Since we're migrating to ember-fetch, we need to ensure it properly handles Basic Authentication headers in the same way as ember-ajax.
✅ Verification successful
Basic Authentication is properly handled by ember-fetch implementation
The codebase shows a proper implementation of Basic Authentication with ember-fetch:
- The NetworkService (using ember-fetch) correctly handles Basic Authentication headers through its
headers()method, which setsAuthorization: 'Basic ' + token - Headers are properly merged with request options in
mergeOptions()method - The
post()method in NetworkService maintains the same behavior as ember-ajax for Basic Authentication
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Basic auth header usage patterns
rg -A 3 "Authorization: \`Basic"
# Search for post operations with custom headers
rg -A 3 "ajax\.post.*headers:"
Length of output: 684
Script:
#!/bin/bash
# Search for ember-fetch imports and usage
rg -A 3 "import fetch"
# Search for fetch related configurations or setup
rg -A 3 "ember-fetch"
# Look for any Basic auth related configurations or middleware
ast-grep --pattern 'class $_ extends $_ {
$$$
headers($$$) {
$$$
}
$$$
}'
Length of output: 12653
app/components/account-settings/general/select-language/index.ts (3)
12-12: Type safety improvement looks good!
The ajax service is now properly typed with IreneAjaxService, improving type safety and maintainability.
Also applies to: 23-23
69-69: Good defensive programming with endpoint fallback!
The addition of the nullish coalescing operator (??) provides a safe fallback for undefined endpoints.
Line range hint 69-77: Verify ember-fetch error handling compatibility
Since we're migrating to ember-fetch, we should verify that the error handling remains compatible, particularly for AdapterError usage.
✅ Verification successful
The previous search revealed extensive usage of AdapterError across the codebase, but we need to verify the error handling compatibility by checking:
- The source of AdapterError and its type definition
- The ajax service implementation to understand the error handling
- Any ember-fetch related imports and usage patterns
This will help us determine if the error handling in the language selection component aligns with the rest of the codebase's error handling patterns.
The previous search revealed that:
- The codebase uses
fetchfrom 'fetch' package in the ajax service - AdapterError is imported from '@ember-data/adapter/error'
- There's a custom ajax service implementation (IreneAjaxService)
We need to examine these implementations to verify the error handling compatibility between ember-fetch and AdapterError usage in the language selection component.
The error handling in the language selection component is compatible with ember-fetch
The code review concern can be resolved based on the following findings:
-
The ajax service (
IreneAjaxService) is already using ember-fetch'sfetchimplementation and properly throws the raw Response object on errors (if (!response.ok) { throw response; }). -
The error handling in the language selection component:
catch (err) {
const error = err as AdapterError;
this.notify.error(error.payload.message);
}is consistent with the DRF adapter's error handling which creates AdapterError instances with the error payload from the server response.
- The
AdapterErrortype is properly imported from '@ember-data/adapter/error', which is the correct package for ember-data error types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for AdapterError usage patterns
rg -A 3 "as AdapterError"
# Search for error handling in post operations
rg -A 5 "ajax\.post.*catch"
Length of output: 26395
Script:
#!/bin/bash
# Search for imports of AdapterError and related error handling types
rg "import.*AdapterError"
# Search for ember-fetch related imports and usage
rg -A 2 "import.*ember-fetch"
rg -A 2 "import.*fetch"
# Search for ajax service imports and definitions
rg "import.*ajax"
rg "class.*AjaxService"
Length of output: 5105
Script:
#!/bin/bash
# Let's examine the ajax service implementation to understand the error handling
cat app/services/ajax.ts
# Let's also check the adapter error definition
cat lib/ember-django-adapter/addon/adapters/drf.js
Length of output: 14839
app/components/project-settings/analysis-settings/toggle-analysis/index.ts (2)
12-12: Type safety improvement looks good
The migration from any to IreneAjaxService type improves type safety. This change aligns with the PR objective of migrating to ember-fetch.
Also applies to: 22-22
Line range hint 47-67: Verify error handling compatibility with ember-fetch
The error handling in toggleUnknownAnalysis task uses parseError utility. Since ember-fetch has different error response structure compared to ember-ajax, ensure that parseError utility is updated to handle fetch responses correctly.
✅ Verification successful
Let me gather more information about the error handling implementation and fetch usage.
Let me check how the ajax service is implemented since it's being used in the component.
Based on the code analysis, I can now provide the final response:
The error handling is compatible with ember-fetch implementation
The parseError utility is already compatible with fetch responses because:
- The ajax service throws the raw Response object on failure (
if (!response.ok) { throw response; }) - The
parseErrorutility handles various error formats including:- Standard error arrays (
err.errors) - HTTP status codes (500, 0)
- Error payloads with detail/message
- Direct error messages/titles
- Standard error arrays (
This flexible error handling structure works with both ember-ajax and ember-fetch response formats.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if parseError utility has been updated for fetch
rg -A 5 "parseError" app/utils/
Length of output: 372
Script:
#!/bin/bash
# Check complete parseError implementation and fetch error handling
cat app/utils/parse-error.ts
# Check for fetch usage and related configurations
rg -l "fetch" app/
rg "ember-fetch" package.json
Length of output: 8719
Script:
#!/bin/bash
# Check ajax service implementation
cat app/services/ajax.ts
# Check for any fetch-specific error handling
rg -A 5 "handleResponse|isSuccess|isError" app/services/
Length of output: 7834
app/components/file-details/static-scan/index.ts (1)
12-12: Type safety improvement looks good
The migration from any to IreneAjaxService type improves type safety. This change aligns with the PR objective of migrating to ember-fetch.
Also applies to: 23-23
app/components/account-settings/developer-settings/personal-token-list/index.ts (3)
12-12: Type safety improvement looks good
The migration from any to IreneAjaxService type improves type safety. This change aligns with the PR objective of migrating to ember-fetch.
Also applies to: 16-16
82-82: Good use of nullish coalescing operator
The use of ?? operator for the endpoint provides a safe fallback, preventing potential issues with undefined endpoints.
Line range hint 1-1: Consider these architectural recommendations for the ember-ajax to ember-fetch migration
- Create a centralized error handling utility that handles both ember-ajax and ember-fetch errors during the migration period
- Update the
IreneAjaxServiceinterface to match ember-fetch's request/response structure - Consider adding type definitions for fetch responses and errors
- Add middleware to handle common response processing (JSON parsing, error handling)
app/components/account-settings/security/password-change/index.ts (1)
17-17: Consider fetch-specific error handling adjustments
While the type change from any to IreneAjaxService is good, since we're migrating to ember-fetch, we should ensure the error handling is compatible with fetch responses rather than ember-ajax's AdapterError.
Let me verify the error handling patterns in other components:
Also applies to: 33-33
app/components/file/report-drawer/sbom-reports/sample/index.ts (1)
13-13: Review parseError utility compatibility
While the type change from any to IreneAjaxService is good, verify if the parseError utility is compatible with fetch responses.
Let me check the parseError implementation:
Also applies to: 21-21
✅ Verification successful
Let me check more context around the parseError implementation and its usage:
Let me check the error handling in the SBOM reports sample component:
Based on the implementation of parseError and its usage in the codebase, along with the ajax service usage in the SBOM reports sample component, I can now provide the final response:
parseError utility is compatible with ajax responses
The parseError utility is designed to handle both standard error objects and error responses with an errors array. Looking at the implementation:
- It handles errors with
err.errorsarray (standard ajax error format) - It processes HTTP status codes (500, 0)
- It extracts error details from
error.payload.detail
Since the component uses ajax.post() which returns standard ajax responses, the error handling with parseError is appropriate and compatible.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for parseError implementation
rg -t ts "function parseError" -A 5
Length of output: 378
Script:
#!/bin/bash
# Get full parseError implementation
rg -t ts "function parseError" -A 15
# Find usages of parseError in the codebase
rg -t ts "parseError\(" -A 2
# Check if there are any fetch-related error handling patterns
rg -t ts "\.catch\(" -A 2 "app/components/file/report-drawer/sbom-reports/sample/"
Length of output: 34553
Script:
#!/bin/bash
# Look for error handling in the SBOM reports sample component
rg -t ts "\.catch\(" -B 5 "app/components/file/report-drawer/sbom-reports/sample/"
# Check ajax service usage in the component
rg -t ts "ajax\." -A 2 "app/components/file/report-drawer/sbom-reports/sample/"
Length of output: 466
app/components/file-details/summary/file-tags/index.ts (1)
12-12: LGTM! Type safety improvement
The change from any to IreneAjaxService type improves type safety and makes the code more maintainable.
Let's verify the ajax method signatures being used:
Also applies to: 22-22
✅ Verification successful
Let me generate another script to search for the interface definition since it wasn't found in the first attempt.
Let me search for the actual implementation of the ajax service to verify the method signatures.
Type change is safe and correctly used
The IreneAjaxService implementation shows that both post and delete methods are available with the correct signatures, accepting a URL and options parameter. The file-tags component correctly uses these methods with the expected parameters including the namespace and data options.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ajax methods used in this component exist in IreneAjaxService
# Search for the IreneAjaxService interface definition
ast-grep --pattern 'interface IreneAjaxService {
$$$
}'
# Search for post and delete method usage patterns
rg -A 2 'ajax\.(post|delete)'
Length of output: 12263
Script:
#!/bin/bash
# Search for IreneAjaxService type definition with different patterns
ast-grep --pattern 'type IreneAjaxService = {
$$$
}'
# Also search for any file containing IreneAjaxService
rg "IreneAjaxService"
Length of output: 8432
Script:
#!/bin/bash
# Search for the ajax service implementation
cat app/services/ajax.ts
Length of output: 7775
app/components/user-registration/via-org-invite/index.ts (1)
15-15: LGTM! Type safety improvement
The change from any to IreneAjaxService type improves type safety and makes the code more maintainable.
Let's verify the ajax method signature being used:
Also applies to: 39-39
✅ Verification successful
Type change is safe and correctly used
The change from any to IreneAjaxService type is correct and safe. The component uses ajax.request() with a POST method, which is fully supported by the IreneAjaxService class through its makeRequest implementation. The service provides proper typing for request options including method and data parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ajax method used in this component exists in IreneAjaxService
# Search for the IreneAjaxService interface definition
ast-grep --pattern 'interface IreneAjaxService {
$$$
}'
# Search for request method usage pattern
rg -A 2 'ajax\.request.*POST'
Length of output: 91
Script:
#!/bin/bash
# Let's try different approaches to find the interface and its usage
# Search for IreneAjaxService definition with type/interface
ast-grep --pattern 'type IreneAjaxService = {
$$$
}'
# Also search for class definition
ast-grep --pattern 'class IreneAjaxService {
$$$
}'
# Search for ajax request usage in the specific file
rg -l "ajax" "app/components/user-registration/via-org-invite/index.ts"
# If found, let's see the content
cat "app/components/user-registration/via-org-invite/index.ts"
# Search for any files containing IreneAjaxService
rg "IreneAjaxService"
Length of output: 12207
Script:
#!/bin/bash
# Let's examine the ajax service implementation to verify the request method
# Get the content of the ajax service
cat "app/services/ajax.ts"
# Also search for any request method usage in the component
rg -A 5 "ajax\.request" "app/components/user-registration/via-org-invite/index.ts"
Length of output: 7977
app/components/project-settings/general-settings/dynamicscan-automation-settings/index.ts (1)
12-12: LGTM! Type safety improvement
The change from any to IreneAjaxService type improves type safety and makes the code more maintainable.
Let's verify the ajax method signature being used:
Also applies to: 25-25
app/components/user-login/reset-password/index.ts (1)
18-18: LGTM: Type safety improvement
Good improvement replacing any with IreneAjaxService type, enhancing type safety and maintainability.
Also applies to: 38-38
app/components/api-filter/index.ts (1)
13-13: LGTM: Type safety improvement
Good improvement replacing any with IreneAjaxService type, enhancing type safety and maintainability.
Also applies to: 33-33
app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts (1)
12-12: LGTM: Type safety improvement
Good improvement replacing any with IreneAjaxService type, enhancing type safety and maintainability.
Also applies to: 23-23
app/components/security/analysis-list/header/index.ts (2)
21-21: LGTM: Type safety improvement
Good improvement replacing any with IreneAjaxService type for better type safety and IDE support.
Also applies to: 43-43
Line range hint 98-112: Verify namespace usage in ajax calls
The ajax call uses a specific namespace 'api/hudson-api'. Ensure this namespace is properly configured in the new fetch service implementation.
✅ Verification successful
Let me check the namespace configuration in the adapters and mirage config to ensure consistency.
The namespace 'api/hudson-api' is properly configured and consistently used across the codebase
The namespace 'api/hudson-api' is correctly implemented:
- It's consistently defined as a base namespace in all security adapters
- The mirage configuration includes routes for the 'api/hudson-api' namespace
- The namespace is consistently used across multiple components for API calls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of this namespace to ensure consistent usage
rg -l "api/hudson-api"
# Search for namespace configurations
rg -l "namespace.*hudson"
Length of output: 1613
Script:
#!/bin/bash
# Check namespace configuration in adapters
rg -A 5 "namespace.*hudson" app/adapters/security/
# Check mirage configuration
cat mirage/config.js
Length of output: 17157
app/components/file-details/dynamic-scan/drawer-old/index.ts (1)
11-11: LGTM: Type safety improvement
Good improvement replacing any with IreneAjaxService type for better type safety and IDE support.
Also applies to: 23-23
app/components/file-details/dynamic-scan/action/drawer/index.ts (2)
20-20: LGTM: Type safety improvement
Good improvement replacing any with IreneAjaxService type for better type safety and IDE support.
Also applies to: 34-34
Line range hint 127-134: Verify API version consistency
The component uses namespace: ENV.namespace_v2 for API calls. Ensure this aligns with the migration to ember-fetch and verify the namespace configuration.
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information about the namespace usage across the codebase.
Namespace configuration is consistent with API v2
The usage of namespace: ENV.namespace_v2 in the component is consistent with the rest of the codebase. The configuration is properly defined in config/environment.js as namespace_v2: 'api/v2' and is used consistently across all API calls in the application, including adapters and components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for namespace_v2 usage
rg "namespace_v2"
# Search for different namespace versions in API calls
rg -A 1 "namespace:.*v[0-9]"
Length of output: 8080
app/components/app-monitoring/version-table/actions/index.ts (3)
20-20: LGTM: Import of IreneAjaxService type
The explicit import of IreneAjaxService type enhances type safety.
31-31: LGTM: Updated ajax service type declaration
The ajax service type has been properly updated from any to IreneAjaxService.
Line range hint 147-166: Verify ember-fetch compatibility with the POST request
The POST request implementation looks correct, but since we're migrating from ember-ajax to ember-fetch, we should verify the endpoint compatibility and response handling.
✅ Verification successful
Let me gather more information about the endpoint usage and response handling.
Let me check the test file and submission model to understand the expected response structure and handling.
POST request implementation is compatible with ember-fetch migration
The implementation is verified to be compatible with ember-fetch based on:
- The endpoint response structure matches the expected type
{ id: number; status: number }as shown in both the component and test files - The test suite confirms proper handling of the POST response and subsequent state management
- The submission model correctly handles the data received from the endpoint
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the endpoint is used elsewhere in the codebase
rg -l "amAppVersions.*initiate_upload" --type ts --type js
# Check the response structure expected by other components
ast-grep --pattern 'const $_ = $_ajax.post($_, $_) as { id: number; status: number; }'
Length of output: 210
Script:
#!/bin/bash
# Check the actual implementation and response handling in the file
cat app/components/app-monitoring/version-table/actions/index.ts
# Look for any test files that might show the expected response structure
fd "actions-test" --type f
# Search for submission model to understand the data structure
fd "submission.ts" --type f
Length of output: 5546
Script:
#!/bin/bash
# Check the test implementation for this component
cat tests/integration/components/app-monitoring/version-table/actions-test.js
# Check the submission model structure
cat app/models/submission.ts
# Look for any mirage mock implementations
cat mirage/factories/submission.ts
Length of output: 17060
app/components/file-details/dynamic-scan/action/drawer/device-pref-table/index.ts (2)
16-16: LGTM: Import of IreneAjaxService type
The explicit import of IreneAjaxService type enhances type safety.
39-39: Verify ajax service usage
While the type update is correct, this component doesn't seem to use the ajax service directly in the provided code.
Consider removing the ajax service injection if it's not used in this component or its children.
✅ Verification successful
Ajax service is actively used in this component
The ajax service is being used in the drawer component to make POST requests, specifically at app/components/file-details/dynamic-scan/action/drawer/index.ts. The service is required and should be kept.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this component or its children use ajax service
rg -l "this\.ajax\." --type ts --type js --glob "app/components/file-details/dynamic-scan/action/drawer/**/*"
Length of output: 175
Script:
#!/bin/bash
# Check the actual usage of ajax service in the drawer component
rg "this\.ajax\." -A 3 --type ts --type js app/components/file-details/dynamic-scan/action/drawer/index.ts
Length of output: 216
app/components/project-settings/general-settings/github-project/index.ts (3)
15-15: LGTM: Import of IreneAjaxService type
The explicit import of IreneAjaxService type enhances type safety.
27-27: LGTM: Updated ajax service type declaration
The ajax service type has been properly updated from any to IreneAjaxService.
Line range hint 164-209: Improve error handling types
The error handling in fetchGithubRepos could be improved with more specific types.
Consider this improvement:
- const error = err as AdapterError;
+ const error = err as AdapterError & {
+ status?: number;
+ payload?: {
+ detail: string;
+ };
+ };Also, verify the compatibility of ajax.request with ember-fetch:
✅ Verification successful
Based on the search results, I can now generate the final response:
Improve error handling types with more specific type definitions
The error handling pattern in fetchGithubRepos aligns with the codebase's common patterns for handling AdapterError. The suggested type improvement would help with type safety. Here's the recommended change:
- const error = err as AdapterError;
+ const error = err as AdapterError & {
+ status?: number;
+ payload?: {
+ detail: string;
+ };
+ };- This matches similar error handling patterns found across the codebase, particularly in components like
organization-team,security, andfile-detailsmodules - The type assertion is safe as it's consistent with how AdapterError is handled in other ajax requests
- The ajax.request endpoint
/api/organizations/${org_id}/github_reposis correctly implemented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other components use similar error handling patterns
rg -A 5 "as AdapterError" --type ts
# Check for other uses of the github repos endpoint
rg "/api/organizations/.*?/github_repos" --type ts --type js
Length of output: 39714
app/components/project-preferences-old/provider/index.ts (1)
18-18: LGTM: Type safety improvement
The change from any to IreneAjaxService type improves type safety and aligns with the migration to ember-fetch.
Also applies to: 47-47
app/components/organization-analytics/app-scan-chart/index.ts (1)
19-19: LGTM: Proper type safety implementation
The changes correctly implement type safety for the ajax service:
- Proper import of IreneAjaxService type
- Correct type declaration for the ajax service
- Well-defined response type with IAppScan interface
Also applies to: 47-47
app/components/project-preferences/provider/index.ts (1)
26-26: LGTM: Proper type safety implementation
The changes correctly implement type safety for the ajax service with proper import and type declaration.
Also applies to: 71-71
Irene
|
||||||||||||||||||||||||||||||
| Project |
Irene
|
| Branch Review |
PD-1583-Remove-Ember-Ajax
|
| Run status |
|
| Run duration | 16m 15s |
| Commit |
|
| Committer | SmitGala |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
3
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
29
|
Upgrade your plan to view test results. | |
| View all changes introduced in this branch ↗︎ | |
Tests for review
SmitGala
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
from
037bf3a to
241788c
Compare
SmitGala
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
from
241788c to
6e29042
Compare
SmitGala
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
from
6e29042 to
eac38b3
Compare
SmitGala
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
from
eac38b3 to
e1ed0b2
Compare
SmitGala
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
from
e1ed0b2 to
f8b239a
Compare
SmitGala
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
from
f8b239a to
2cdeec2
Compare
SmitGala
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
from
2cdeec2 to
35d0630
Compare
SmitGala
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
from
35d0630 to
3ba314c
Compare
future-pirate-king
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
from
3ba314c to
b5d5b99
Compare
future-pirate-king
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
from
b5d5b99 to
650d060
Compare
SmitGala
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
2 times, most recently
from
4bd3110 to
2965f95
Compare
SmitGala
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
from
67cbaac to
4739cf0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/components/account-settings/security/multi-factor-auth/index.ts (1)
527-527:⚠️ Potential issueConsider removing or masking tokens from debug logs.
Logging tokens, even in debug mode, can be a security concern. Consider masking or removing sensitive data from logs.
- debug('SwitchTOEmail: App OTP Token Data ' + tokenData?.token); + debug('SwitchTOEmail: App OTP Token Data [MASKED]');
🧹 Nitpick comments (9)
tests/integration/components/file-details/dynamic-scan/manual-test.js (2)
1013-1013: Verify device type enum consistency.The code uses magic numbers (
ENUMS.DEVICE_TYPE.PHONE_REQUIRED) in multiple places. While the enum values are correct, consider extracting these into constants at the top of the file to improve maintainability.+ const DEVICE_TYPES = { + PHONE_REQUIRED: ENUMS.DEVICE_TYPE.PHONE_REQUIRED, + TABLET_REQUIRED: ENUMS.DEVICE_TYPE.TABLET_REQUIRED, + NO_PREFERENCE: ENUMS.DEVICE_TYPE.NO_PREFERENCE + };Also applies to: 1627-1627
Line range hint
1599-1616: Enhance test assertions for device preferences.The test verifies device preference reset functionality but could be more explicit about the expected behavior. Consider adding assertions to verify:
- The timing of the preference reset
- The completeness of the reset
Add assertions to verify the timing and completeness:
if (this.checkPreferenceReset) { const windowService = this.owner.lookup('service:browser/window'); + assert.ok( + this.file.isDynamicStatusInProgress, + 'Preferences should only reset when scan is in progress' + ); const actualDevicePrefData = JSON.parse( windowService.localStorage.getItem('actualDevicePrefData') ); assert.strictEqual( data.device_type, actualDevicePrefData.device_type ); assert.strictEqual( data.platform_version, String(actualDevicePrefData.platform_version) ); + assert.deepEqual( + Object.keys(actualDevicePrefData), + ['device_type', 'platform_version'], + 'Should only contain expected preference keys' + ); }app/components/public-api-docs/index.ts (1)
81-84: Consider enhancing type safety and request specificity.While the changes align with the migration to ember-fetch, consider these improvements:
- Type-cast the error for consistent error handling across the codebase
- Consider using a more specific accept header
- Validate the response structure matches SwaggerUIDataProps
fetchSchemaData = task(async () => { try { this.data = await waitForPromise( this.ajax.request<SwaggerUIDataProps>('/api/public_api/schema', { - headers: { accept: 'application/json, */*' }, + headers: { accept: 'application/json' }, }) ); - } catch (error) { + } catch (error: unknown) { + const ajaxError = error as AjaxError; this.notify.error(parseError(error, this.intl.t('pleaseTryAgain'))); } });app/components/user-registration/via-login-page/index.ts (1)
Line range hint
91-117: Consider adding tests for the new ajax service integration.While the changes look good, it would be beneficial to add tests that verify:
- The new ajax service integration works as expected
- Error handling properly handles different types of AjaxError responses
- The data wrapping doesn't break the server communication
🧰 Tools
🪛 Biome (1.9.4)
[error] 99-99: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/project-settings/analysis-settings/vulnerability-list/index.ts (1)
Line range hint
182-198: Consider adding error handling to the save task.While the type safety improvements are good, the task should include error handling similar to
fetchVulnerabilityPreferences.Consider adding a try-catch block:
saveVulnerabilityPreferences = task( async (data: { risk: number | string; comment: string }) => { + try { const profileId = this.project?.activeProfileId; const vulnerabilityId = this.vulnerability?.id; const url = [ ENV.endpoints['profiles'], profileId, ENV.endpoints['vulnerabilityPreferences'], vulnerabilityId, ].join('/'); const formData = buildURLEncodedFormData(data); const res = await this.ajax.put<VulnerabilityPreferenceResponse>(url, { data: formData, contentType: 'application/x-www-form-urlencoded', }); // update model this.selectedVulnerabilityPreference?.updateValues(res); + } catch (error) { + this.notify.error(parseError(error, this.intl.t('pleaseTryAgain'))); + throw error; + } } );app/services/ajax.ts (4)
33-41: Consider using a more specific type for error payloadThe
payloadproperty inAjaxErrorinterface usesanytype, which bypasses TypeScript's type checking. Consider using a generic type parameter or a more specific type to maintain type safety.-export interface AjaxError { +export interface AjaxError<T = unknown> { type: string; status: number; ok: boolean; statusText: string; url: string; message: string; - payload: any; + payload: T; }
95-108: Consider using URL API instead of DOM manipulationUsing DOM manipulation for URL parsing is browser-dependent and less efficient. The URL API provides a more robust solution.
export function parseURL(str: string) { - const element = document.createElement('a'); - element.href = str; + const url = new URL(str, window.location.origin); return { - href: element.href, - protocol: element.protocol, - hostname: element.hostname, - port: element.port, - pathname: element.pathname, - search: element.search, - hash: element.hash, + href: url.href, + protocol: url.protocol, + hostname: url.hostname, + port: url.port, + pathname: url.pathname, + search: url.search, + hash: url.hash, }; }
249-253: Cache compiled RegExp patternsCreating new RegExp instances for each hostname match can be inefficient. Consider caching the compiled patterns.
+ private patternCache = new Map<string, RegExp>(); + private _matchHosts(hostname: string, pattern: string): boolean { - const regexPattern = pattern.replace(/\./g, '\\.').replace(/\*/g, '.*'); - return new RegExp(`^${regexPattern}$`).test(hostname); + let regex = this.patternCache.get(pattern); + if (!regex) { + const regexPattern = pattern.replace(/\./g, '\\.').replace(/\*/g, '.*'); + regex = new RegExp(`^${regexPattern}$`); + this.patternCache.set(pattern, regex); + } + return regex.test(hostname); }
262-299: Consider implementing request timeoutThe
makeRequestmethod doesn't handle request timeouts. Consider adding a timeout option to prevent long-running requests.interface RequestOptions extends RequestInit { + /** Optional timeout in milliseconds */ + timeout?: number; } async makeRequest<T>(url: string, options: RequestOptions = {}): Promise<T> { const finalUrl = this._buildURL(url, options); + const controller = new AbortController(); + const { signal } = controller; + + const timeout = options.timeout ?? 30000; // Default 30s timeout + const timeoutId = setTimeout(() => controller.abort(), timeout); const fetchOptions: RequestInit = { ...options, headers, body: this.processRequestBody(options.data), + signal, }; (fetchOptions as RequestOptions).namespace = undefined; - const response = await fetch(finalUrl, fetchOptions); + try { + const response = await fetch(finalUrl, fetchOptions); + clearTimeout(timeoutId); // ... rest of the code + } catch (error) { + clearTimeout(timeoutId); + if (error.name === 'AbortError') { + throw new Error(`Request timeout after ${timeout}ms`); + } + throw error; + } }🧰 Tools
🪛 Biome (1.9.4)
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (82)
app/adapters/file-report.ts(1 hunks)app/authenticators/base.ts(2 hunks)app/authenticators/irene.ts(1 hunks)app/authenticators/saml2.ts(1 hunks)app/components/account-settings/developer-settings/personal-token-list/delete-token/index.ts(3 hunks)app/components/account-settings/developer-settings/personal-token-list/index.ts(3 hunks)app/components/account-settings/general/select-language/index.ts(3 hunks)app/components/account-settings/security/multi-factor-auth/index.ts(16 hunks)app/components/account-settings/security/password-change/index.ts(4 hunks)app/components/analysis-risk/override-edit-drawer/override-form/index.ts(0 hunks)app/components/api-filter/index.ts(3 hunks)app/components/app-monitoring/version-table/actions/index.ts(2 hunks)app/components/appknox-wrapper/index.ts(0 hunks)app/components/attachment-detail/index.ts(1 hunks)app/components/dynamicscan-automation-upselling-feature/index.ts(1 hunks)app/components/file-details/api-scan/captured-apis/footer/index.ts(2 hunks)app/components/file-details/api-scan/captured-apis/index.ts(3 hunks)app/components/file-details/dynamic-scan/action/drawer/device-pref-table/index.ts(0 hunks)app/components/file-details/dynamic-scan/action/drawer/index.ts(2 hunks)app/components/file-details/dynamic-scan/action/index.ts(3 hunks)app/components/file-details/dynamic-scan/drawer-old/index.ts(2 hunks)app/components/file-details/dynamic-scan/results/index.ts(0 hunks)app/components/file-details/manual-scan/index.ts(3 hunks)app/components/file-details/manual-scan/request-access/index.ts(2 hunks)app/components/file-details/static-scan/index.ts(3 hunks)app/components/file-details/summary/file-tags/index.ts(2 hunks)app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts(2 hunks)app/components/file-details/vulnerability-analysis/header/index.ts(2 hunks)app/components/file/report-drawer/sbom-reports/sample/index.ts(1 hunks)app/components/home-page/index.ts(0 hunks)app/components/organization-analytics/app-scan-chart/index.ts(2 hunks)app/components/organization-analytics/index.ts(2 hunks)app/components/organization-billing/invoice-list/download-action/index.ts(2 hunks)app/components/organization-billing/subscription/index.ts(3 hunks)app/components/organization-invitation-list/invite-delete/index.ts(1 hunks)app/components/organization-invitation-list/invite-resend/index.ts(0 hunks)app/components/organization-team/create-team/index.ts(0 hunks)app/components/project-preferences-old/provider/index.ts(2 hunks)app/components/project-preferences/provider/index.ts(2 hunks)app/components/project-settings/analysis-settings/toggle-analysis/index.ts(2 hunks)app/components/project-settings/analysis-settings/vulnerability-list/index.ts(4 hunks)app/components/project-settings/general-settings/dynamicscan-automation-settings/index.ts(2 hunks)app/components/project-settings/general-settings/github-project/index.ts(3 hunks)app/components/project-settings/view-scenario/index.ts(0 hunks)app/components/public-api-docs/index.ts(2 hunks)app/components/security/analysis-details/attachments/index.ts(4 hunks)app/components/security/analysis-details/index.ts(2 hunks)app/components/security/analysis-list/header/index.ts(2 hunks)app/components/security/analysis-list/table/action/index.ts(2 hunks)app/components/security/analysis-report-btn/index.ts(3 hunks)app/components/security/download-app/index.ts(1 hunks)app/components/security/file-details-actions/index.ts(4 hunks)app/components/security/file-search-list/download/index.ts(2 hunks)app/components/security/purge-api-analysis/index.ts(1 hunks)app/components/system-status/index.ts(3 hunks)app/components/user-login/index.ts(3 hunks)app/components/user-login/recover-password/index.ts(3 hunks)app/components/user-login/reset-password/index.ts(3 hunks)app/components/user-registration/via-login-page/index.ts(3 hunks)app/components/user-registration/via-org-invite/index.ts(3 hunks)app/components/user-registration/via-partner-invite/index.ts(4 hunks)app/routes/authenticated/dashboard/organization-settings/index.ts(1 hunks)app/routes/authenticated/github-cloud/redirect.ts(2 hunks)app/routes/authenticated/payment-success.ts(1 hunks)app/routes/invite.ts(1 hunks)app/services/ajax.js(0 hunks)app/services/ajax.ts(1 hunks)app/services/configuration.ts(6 hunks)app/services/devicefarm.ts(2 hunks)app/services/freshdesk.ts(3 hunks)app/services/oidc.ts(5 hunks)app/services/organization.ts(1 hunks)app/services/websocket.ts(3 hunks)package.json(0 hunks)tests/integration/components/file-details/dynamic-scan/manual-test.js(5 hunks)tests/integration/components/file-details/summary-test.js(1 hunks)tests/integration/components/file-details/vulnerability-analysis-details/edit-analysis-button-test.js(2 hunks)tests/integration/components/file/report-drawer/va-reports/report-item-test.js(3 hunks)tests/integration/components/partner/client-report-download-test.js(3 hunks)tests/integration/components/project-settings/analysis-settings/vulnerability-list/index-test.js(3 hunks)tests/integration/components/project-settings/general-settings/dynamicscan-automation-settings/index-test.js(1 hunks)tests/integration/components/security/analysis-details-test.js(2 hunks)
⛔ Files not processed due to max files limit (6)
- tests/integration/components/user-login/recover-password-test.js
- tests/integration/components/via-org-invite-test.js
- tests/integration/components/via-partner-invite-test.js
- tests/unit/services/ajax-test.js
- tsconfig.json
- types/irene/index.d.ts
💤 Files with no reviewable changes (10)
- app/components/analysis-risk/override-edit-drawer/override-form/index.ts
- package.json
- app/services/ajax.js
- app/components/project-settings/view-scenario/index.ts
- app/components/organization-team/create-team/index.ts
- app/components/file-details/dynamic-scan/results/index.ts
- app/components/file-details/dynamic-scan/action/drawer/device-pref-table/index.ts
- app/components/appknox-wrapper/index.ts
- app/components/home-page/index.ts
- app/components/organization-invitation-list/invite-resend/index.ts
🚧 Files skipped from review as they are similar to previous changes (63)
- tests/integration/components/project-settings/analysis-settings/vulnerability-list/index-test.js
- app/components/organization-invitation-list/invite-delete/index.ts
- app/components/project-preferences/provider/index.ts
- app/services/organization.ts
- tests/integration/components/security/analysis-details-test.js
- tests/integration/components/file/report-drawer/va-reports/report-item-test.js
- app/components/file-details/dynamic-scan/drawer-old/index.ts
- app/components/organization-analytics/app-scan-chart/index.ts
- app/routes/invite.ts
- app/components/app-monitoring/version-table/actions/index.ts
- app/components/project-preferences-old/provider/index.ts
- app/components/security/analysis-list/table/action/index.ts
- tests/integration/components/project-settings/general-settings/dynamicscan-automation-settings/index-test.js
- app/components/security/analysis-details/attachments/index.ts
- app/services/websocket.ts
- app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts
- app/components/file-details/api-scan/captured-apis/footer/index.ts
- app/components/dynamicscan-automation-upselling-feature/index.ts
- app/components/file-details/dynamic-scan/action/drawer/index.ts
- app/components/user-registration/via-org-invite/index.ts
- app/services/freshdesk.ts
- app/components/api-filter/index.ts
- app/components/user-login/recover-password/index.ts
- app/routes/authenticated/github-cloud/redirect.ts
- app/components/file-details/dynamic-scan/action/index.ts
- app/components/system-status/index.ts
- app/routes/authenticated/dashboard/organization-settings/index.ts
- app/components/security/analysis-details/index.ts
- app/services/oidc.ts
- app/components/file-details/static-scan/index.ts
- app/components/file-details/summary/file-tags/index.ts
- app/components/user-registration/via-partner-invite/index.ts
- app/components/organization-billing/subscription/index.ts
- app/components/attachment-detail/index.ts
- app/components/project-settings/general-settings/github-project/index.ts
- app/components/account-settings/general/select-language/index.ts
- app/components/security/download-app/index.ts
- app/components/user-login/reset-password/index.ts
- app/components/security/file-search-list/download/index.ts
- app/adapters/file-report.ts
- app/authenticators/base.ts
- app/authenticators/irene.ts
- app/components/security/purge-api-analysis/index.ts
- app/components/organization-analytics/index.ts
- app/authenticators/saml2.ts
- app/components/project-settings/general-settings/dynamicscan-automation-settings/index.ts
- app/components/account-settings/security/password-change/index.ts
- app/components/file-details/manual-scan/index.ts
- app/services/devicefarm.ts
- app/components/security/analysis-list/header/index.ts
- app/components/security/file-details-actions/index.ts
- app/components/file-details/manual-scan/request-access/index.ts
- app/components/project-settings/analysis-settings/toggle-analysis/index.ts
- app/routes/authenticated/payment-success.ts
- app/components/file-details/api-scan/captured-apis/index.ts
- app/components/security/analysis-report-btn/index.ts
- app/components/account-settings/developer-settings/personal-token-list/delete-token/index.ts
- app/components/file/report-drawer/sbom-reports/sample/index.ts
- app/components/account-settings/developer-settings/personal-token-list/index.ts
- tests/integration/components/file-details/summary-test.js
- tests/integration/components/partner/client-report-download-test.js
- app/components/file-details/vulnerability-analysis/header/index.ts
- tests/integration/components/file-details/vulnerability-analysis-details/edit-analysis-button-test.js
🧰 Additional context used
🪛 Biome (1.9.4)
app/services/ajax.ts
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (23)
tests/integration/components/file-details/dynamic-scan/manual-test.js (1)
109-109: Improved request body handling.The change from string parsing to JSON parsing aligns with the migration from
ember-ajaxtoember-fetch. This is a more robust approach as it properly handles the JSON request body.app/components/public-api-docs/index.ts (3)
14-14: LGTM! Clean type import.The import statement correctly uses the
typekeyword for TypeScript type imports.
21-21: LGTM! Proper service declaration.The service declaration correctly uses TypeScript's
declarekeyword and is properly typed withIreneAjaxService.
81-84: Verify consistent error handling across components.The AI summary mentions that other components have been updated to use
AjaxErrortype for error handling. Let's verify this pattern is consistently applied.✅ Verification successful
Error handling pattern is consistent with codebase conventions.
The file uses the standard
parseErrorutility with fallback message, which aligns with the dominant error handling pattern across components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling patterns in components using ajax service rg -A 2 "catch \(error" "app/components/"Length of output: 30520
app/components/user-registration/via-login-page/index.ts (4)
16-17: LGTM! Type imports are well-structured.The type imports are correctly defined and follow TypeScript best practices.
39-39: LGTM! Service injection properly typed.The service injection is correctly updated to use IreneAjaxService, aligning with the PR's objective of replacing ember-ajax.
92-92: Verify consistent usage of ajax.post across the codebase.The change from
network.posttoajax.postincludes a modification in the payload structure (wrapping data in adataobject). Let's verify this pattern is consistently applied across all ajax calls.✅ Verification successful
All ajax.post calls consistently wrap payload data
The codebase shows consistent usage of the ajax.post API. All instances that send data properly wrap it in a
dataobject, while endpoints that don't require data either omit the payload or use other specific properties likenamespace.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all ajax.post calls to ensure consistent data wrapping rg -t ts -t js "ajax\.post\(" -A 2 -B 2Length of output: 11623
96-96: Verify AjaxError structure compatibility.The error handling assumes that
AjaxErrorhas a similar structure to the previousAdapterError. Let's verify the error interface compatibility across the codebase.✅ Verification successful
AjaxError structure is compatible with error handling.
The error handling is consistent with the codebase-wide patterns. The AjaxError interface provides all necessary properties (status, message, payload) used throughout the application, and the current implementation follows the established error handling conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for AjaxError usage patterns ast-grep --pattern 'interface AjaxError { $$$ }' # Find all error handling patterns rg -t ts "as AjaxError" -A 5 -B 5Length of output: 27564
app/components/project-settings/analysis-settings/vulnerability-list/index.ts (3)
18-19: LGTM! Well-structured type definitions.The new imports and the
VulnerabilityPreferenceResponseinterface provide proper type safety for the ajax service transition.Also applies to: 32-38
44-44: LGTM! Type-safe service injection.The change from
anytoIreneAjaxServiceimproves type safety and aligns with the ember-ajax replacement objective.
174-175: Verify backend handling of empty strings.The change from
nullto empty strings forriskandcommentduring reset might affect backend behavior.Let's verify how the backend handles empty strings vs null values:
app/services/ajax.ts (1)
280-280: Replace delete operator with undefined assignmentUsing the delete operator can impact performance. Consider using undefined assignment instead.
- delete (fetchOptions as RequestOptions).namespace; + (fetchOptions as RequestOptions).namespace = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
app/components/organization-billing/invoice-list/download-action/index.ts (2)
15-17: LGTM! Type definition improves response handling.The
DownloadURLResponsetype properly defines the expected structure of the AJAX response.
29-29: LGTM! Type-safe AJAX request.The AJAX request now uses the
DownloadURLResponsetype, ensuring type safety for the response handling.app/services/configuration.ts (4)
20-24: LGTM! Comprehensive type definitions enhance type safety.The new type definitions (
ImageData,IntegrationData,ThemeData,FrontendConfigResponse,ServerResponse,DashboardResponse) provide clear structure for configuration data, improving maintainability and type safety.Also applies to: 26-35, 37-43, 45-57, 59-64, 66-69
132-134: LGTM! Type-safe frontend configuration request.The AJAX request now uses the
FrontendConfigResponsetype, ensuring type-safe handling of the configuration data.
204-206: LGTM! Type-safe server configuration request.The AJAX request now uses the
ServerResponsetype, ensuring type-safe handling of the server configuration data.
228-230: LGTM! Type-safe dashboard configuration request.The AJAX request now uses the
DashboardResponsetype, ensuring type-safe handling of the dashboard configuration data.app/components/account-settings/security/multi-factor-auth/index.ts (2)
23-26: LGTM! Type definition improves token handling.The
TokenDatatype properly defines the structure of MFA tokens.
743-743: Type assertion could be dangerous.The type assertion
as TokenDatamight hide potential runtime errors if the response doesn't match the expected structure. Consider adding runtime validation.Also applies to: 755-757
app/components/user-login/index.ts (3)
12-12: Well-structured type definitions for SSO operations!The new types
SSOCheckDataandSSOSaml2Dataprovide good type safety for the SSO operations and match the expected API response structure.Also applies to: 16-24
31-31: LGTM: Service properly typed with IreneAjaxServiceThe ajax service is correctly typed, aligning with the migration from ember-ajax to ember-fetch.
168-168: 🛠️ Refactor suggestionAdd error handling for network failures
While the request is properly typed with
SSOSaml2Data, it lacks error handling for network failures. Consider wrapping it in a try-catch block:- const data = await this.ajax.request<SSOSaml2Data>(url); + try { + const data = await this.ajax.request<SSOSaml2Data>(url); + + if (!data.url) { + this.logger.error('Invalid sso redirect call', data); + return; + } + + this.window.location.href = data.url; + } catch (error) { + this.logger.error('SSO login failed', error); + this.notifications.error( + this.intl.t('ssoLoginFailed'), + ENV.notifications + ); + } - - if (!data.url) { - this.logger.error('Invalid sso redirect call', data); - - return; - } - - this.window.location.href = data.url;Likely invalid or redundant comment.
SmitGala
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
from
4739cf0 to
bbd717a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/components/account-settings/security/multi-factor-auth/index.ts (1)
527-527:⚠️ Potential issueConsider removing or masking tokens from debug logs.
Logging tokens, even in debug mode, can be a security concern. Consider masking sensitive data from logs.
- debug('SwitchTOEmail: App OTP Token Data ' + tokenData?.token); + debug('SwitchTOEmail: App OTP Token Data [MASKED]');app/components/user-login/index.ts (1)
168-168:⚠️ Potential issueAdd error handling for network failures.
The SSO login request lacks error handling for network failures, which could lead to unhandled promise rejections and poor user experience.
Add a try-catch block:
ssologinTask = task(async () => { const return_to = this.samlRedirectURL; const token = this.checkToken; const endpoint = this.SSOAuthenticateEndpoint; const url = `${endpoint}?token=${token}&return_to=${return_to}`; - const data = await this.ajax.request<SSOSaml2Data>(url); + try { + const data = await this.ajax.request<SSOSaml2Data>(url); + + if (!data.url) { + this.logger.error('Invalid sso redirect call', data); + return; + } + + this.window.location.href = data.url; + } catch (error) { + this.logger.error('SSO login failed', error); + this.notifications.error( + this.intl.t('ssoLoginFailed'), + ENV.notifications + ); + } - - if (!data.url) { - this.logger.error('Invalid sso redirect call', data); - return; - } - - this.window.location.href = data.url; });app/services/oidc.ts (1)
119-121:⚠️ Potential issueAdd URL validation before redirects.
The redirect URL should be validated before navigation to prevent potential security risks.
This is a duplicate of a previous review comment suggesting the implementation of URL validation before performing redirects.
🧹 Nitpick comments (7)
app/components/account-settings/security/multi-factor-auth/index.ts (1)
755-757: Fix type assertion.The type assertion
as TokenDatais unnecessary sincestaVerifyEmailOTPis already typed to returnTokenData. Consider removing the type assertion for cleaner code.- tokenData = (await this.staVerifyEmailOTP.perform( - emailOTPData.otp - )) as TokenData; + tokenData = await this.staVerifyEmailOTP.perform(emailOTPData.otp);app/components/user-login/index.ts (1)
12-12: LGTM! Consider adding runtime validation for SSO data.The new type definitions improve type safety for SSO operations. Consider adding runtime validation using a library like
zodto ensure the API responses match these types.Also applies to: 16-24
app/services/configuration.ts (2)
20-24: LGTM! Well-structured type definitions.The new TypeScript types provide good type safety and documentation for the configuration data structure.
Consider making some fields optional with
?if they can be empty, and using literal types for fields with known values (e.g.,scheme: 'light' | 'dark').Also applies to: 26-35, 37-43
Line range hint
131-162: Consider improving error handling and value assignments.
- Error handling could be enhanced:
- Currently swallowing errors after logging
- No retry mechanism for failed requests
- The
||=operator might prevent falsy values from being set. For example, if the API explicitly returnsfalseforhide_poweredby_logo, it won't override the default value.Consider these improvements:
async frontendConfigFetch() { try { const data = await this.ajax.request<FrontendConfigResponse>( this.frontendConfigEndpoint ); - this.frontendData.hide_poweredby_logo = data.hide_poweredby_logo == true; - this.frontendData.name ||= data.name; + // Use direct assignment to allow all valid values from API + this.frontendData = { + ...this.frontendData, + hide_poweredby_logo: data.hide_poweredby_logo === true, + name: data.name, + // ... other properties + }; } catch (error) { this.logger.error('Error getting frontend configuration', error); + // Either re-throw or return a status + throw new Error('Failed to fetch frontend configuration'); } }Also applies to: 203-213, 227-235
app/services/ajax.ts (1)
262-299: Consider adding request timeout.The
makeRequestmethod should include a timeout to prevent hanging requests.async makeRequest<T>(url: string, options: RequestOptions = {}): Promise<T> { const finalUrl = this._buildURL(url, options); const headers = this._shouldSendHeaders(finalUrl, options.host) ? { 'Content-Type': options.contentType || 'application/json', ...this.defaultHeaders, ...options.headers, } : options.headers || {}; const fetchOptions: RequestInit = { ...options, headers, body: this.processRequestBody(options.data), + signal: AbortSignal.timeout(30000), // 30 seconds timeout }; (fetchOptions as RequestOptions).namespace = undefined; const response = await fetch(finalUrl, fetchOptions);🧰 Tools
🪛 Biome (1.9.4)
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
app/services/oidc.ts (1)
Line range hint
64-168: Implement consistent error handling patterns across all methods.While the transition to
IreneAjaxServiceis good, there are consistent patterns that need improvement across all methods:
- Error type assertions should use
Partial<T>- All redirects need URL validation
- Error handling should use a proper Error class
- Null checks should be added for all property accesses
Consider creating shared utility functions for:
- URL validation
- Error handling
- Redirect handling
This will improve code maintainability and security across all OIDC-related operations.
app/components/project-settings/analysis-settings/vulnerability-list/index.ts (1)
174-175: Consider using null or undefined for reset values.Empty strings might not be the most semantically appropriate values for resetting preferences. Consider using
nullorundefinedto explicitly indicate the absence of a value, which better represents a reset state.- risk: '', - comment: '', + risk: null, + comment: null
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (82)
app/adapters/file-report.ts(1 hunks)app/authenticators/base.ts(2 hunks)app/authenticators/irene.ts(1 hunks)app/authenticators/saml2.ts(1 hunks)app/components/account-settings/developer-settings/personal-token-list/delete-token/index.ts(3 hunks)app/components/account-settings/developer-settings/personal-token-list/index.ts(3 hunks)app/components/account-settings/general/select-language/index.ts(3 hunks)app/components/account-settings/security/multi-factor-auth/index.ts(16 hunks)app/components/account-settings/security/password-change/index.ts(4 hunks)app/components/analysis-risk/override-edit-drawer/override-form/index.ts(0 hunks)app/components/api-filter/index.ts(3 hunks)app/components/app-monitoring/version-table/actions/index.ts(2 hunks)app/components/appknox-wrapper/index.ts(0 hunks)app/components/attachment-detail/index.ts(1 hunks)app/components/dynamicscan-automation-upselling-feature/index.ts(1 hunks)app/components/file-details/api-scan/captured-apis/footer/index.ts(2 hunks)app/components/file-details/api-scan/captured-apis/index.ts(3 hunks)app/components/file-details/dynamic-scan/action/drawer/device-pref-table/index.ts(0 hunks)app/components/file-details/dynamic-scan/action/drawer/index.ts(2 hunks)app/components/file-details/dynamic-scan/action/index.ts(3 hunks)app/components/file-details/dynamic-scan/drawer-old/index.ts(2 hunks)app/components/file-details/dynamic-scan/results/index.ts(0 hunks)app/components/file-details/manual-scan/index.ts(3 hunks)app/components/file-details/manual-scan/request-access/index.ts(2 hunks)app/components/file-details/static-scan/index.ts(3 hunks)app/components/file-details/summary/file-tags/index.ts(2 hunks)app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts(2 hunks)app/components/file-details/vulnerability-analysis/header/index.ts(2 hunks)app/components/file/report-drawer/sbom-reports/sample/index.ts(1 hunks)app/components/home-page/index.ts(0 hunks)app/components/organization-analytics/app-scan-chart/index.ts(2 hunks)app/components/organization-analytics/index.ts(2 hunks)app/components/organization-billing/invoice-list/download-action/index.ts(2 hunks)app/components/organization-billing/subscription/index.ts(3 hunks)app/components/organization-invitation-list/invite-delete/index.ts(1 hunks)app/components/organization-invitation-list/invite-resend/index.ts(0 hunks)app/components/organization-team/create-team/index.ts(0 hunks)app/components/project-preferences-old/provider/index.ts(2 hunks)app/components/project-preferences/provider/index.ts(2 hunks)app/components/project-settings/analysis-settings/toggle-analysis/index.ts(2 hunks)app/components/project-settings/analysis-settings/vulnerability-list/index.ts(4 hunks)app/components/project-settings/general-settings/dynamicscan-automation-settings/index.ts(2 hunks)app/components/project-settings/general-settings/github-project/index.ts(3 hunks)app/components/project-settings/view-scenario/index.ts(0 hunks)app/components/public-api-docs/index.ts(2 hunks)app/components/security/analysis-details/attachments/index.ts(4 hunks)app/components/security/analysis-details/index.ts(2 hunks)app/components/security/analysis-list/header/index.ts(2 hunks)app/components/security/analysis-list/table/action/index.ts(2 hunks)app/components/security/analysis-report-btn/index.ts(3 hunks)app/components/security/download-app/index.ts(1 hunks)app/components/security/file-details-actions/index.ts(4 hunks)app/components/security/file-search-list/download/index.ts(2 hunks)app/components/security/purge-api-analysis/index.ts(1 hunks)app/components/system-status/index.ts(3 hunks)app/components/user-login/index.ts(3 hunks)app/components/user-login/recover-password/index.ts(3 hunks)app/components/user-login/reset-password/index.ts(3 hunks)app/components/user-registration/via-login-page/index.ts(3 hunks)app/components/user-registration/via-org-invite/index.ts(3 hunks)app/components/user-registration/via-partner-invite/index.ts(4 hunks)app/routes/authenticated/dashboard/organization-settings/index.ts(1 hunks)app/routes/authenticated/github-cloud/redirect.ts(2 hunks)app/routes/authenticated/payment-success.ts(1 hunks)app/routes/invite.ts(1 hunks)app/services/ajax.js(0 hunks)app/services/ajax.ts(1 hunks)app/services/configuration.ts(6 hunks)app/services/devicefarm.ts(2 hunks)app/services/freshdesk.ts(3 hunks)app/services/oidc.ts(5 hunks)app/services/organization.ts(1 hunks)app/services/websocket.ts(3 hunks)package.json(0 hunks)tests/integration/components/file-details/dynamic-scan/manual-test.js(5 hunks)tests/integration/components/file-details/summary-test.js(1 hunks)tests/integration/components/file-details/vulnerability-analysis-details/edit-analysis-button-test.js(2 hunks)tests/integration/components/file/report-drawer/va-reports/report-item-test.js(3 hunks)tests/integration/components/partner/client-report-download-test.js(3 hunks)tests/integration/components/project-settings/general-settings/dynamicscan-automation-settings/index-test.js(1 hunks)tests/integration/components/security/analysis-details-test.js(2 hunks)tests/integration/components/user-login/recover-password-test.js(2 hunks)
⛔ Files not processed due to max files limit (5)
- tests/integration/components/via-org-invite-test.js
- tests/integration/components/via-partner-invite-test.js
- tests/unit/services/ajax-test.js
- tsconfig.json
- types/irene/index.d.ts
💤 Files with no reviewable changes (10)
- app/components/project-settings/view-scenario/index.ts
- app/components/file-details/dynamic-scan/results/index.ts
- app/components/analysis-risk/override-edit-drawer/override-form/index.ts
- app/components/home-page/index.ts
- app/components/organization-team/create-team/index.ts
- app/services/ajax.js
- app/components/organization-invitation-list/invite-resend/index.ts
- package.json
- app/components/file-details/dynamic-scan/action/drawer/device-pref-table/index.ts
- app/components/appknox-wrapper/index.ts
🚧 Files skipped from review as they are similar to previous changes (62)
- app/authenticators/base.ts
- app/services/organization.ts
- app/components/organization-invitation-list/invite-delete/index.ts
- app/authenticators/irene.ts
- app/routes/invite.ts
- app/components/security/file-search-list/download/index.ts
- app/components/security/download-app/index.ts
- app/components/security/purge-api-analysis/index.ts
- app/components/project-preferences/provider/index.ts
- tests/integration/components/security/analysis-details-test.js
- app/components/security/analysis-list/header/index.ts
- tests/integration/components/project-settings/general-settings/dynamicscan-automation-settings/index-test.js
- app/components/organization-billing/invoice-list/download-action/index.ts
- app/components/organization-billing/subscription/index.ts
- app/services/devicefarm.ts
- app/components/app-monitoring/version-table/actions/index.ts
- app/components/project-settings/general-settings/dynamicscan-automation-settings/index.ts
- app/components/file-details/static-scan/index.ts
- app/components/public-api-docs/index.ts
- app/components/file-details/vulnerability-analysis/header/index.ts
- app/components/project-preferences-old/provider/index.ts
- app/components/dynamicscan-automation-upselling-feature/index.ts
- app/routes/authenticated/dashboard/organization-settings/index.ts
- app/components/file-details/dynamic-scan/action/drawer/index.ts
- app/components/file-details/dynamic-scan/action/index.ts
- app/components/project-settings/analysis-settings/toggle-analysis/index.ts
- app/components/attachment-detail/index.ts
- app/authenticators/saml2.ts
- app/components/security/analysis-details/index.ts
- app/components/api-filter/index.ts
- app/components/account-settings/developer-settings/personal-token-list/delete-token/index.ts
- app/components/file-details/manual-scan/index.ts
- app/components/file-details/summary/file-tags/index.ts
- tests/integration/components/file-details/dynamic-scan/manual-test.js
- app/routes/authenticated/payment-success.ts
- app/components/account-settings/developer-settings/personal-token-list/index.ts
- app/components/user-login/recover-password/index.ts
- app/components/user-login/reset-password/index.ts
- app/components/file/report-drawer/sbom-reports/sample/index.ts
- app/components/account-settings/security/password-change/index.ts
- app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts
- tests/integration/components/file/report-drawer/va-reports/report-item-test.js
- app/adapters/file-report.ts
- app/components/account-settings/general/select-language/index.ts
- app/components/organization-analytics/app-scan-chart/index.ts
- app/components/file-details/dynamic-scan/drawer-old/index.ts
- app/components/user-registration/via-partner-invite/index.ts
- app/components/file-details/api-scan/captured-apis/footer/index.ts
- app/routes/authenticated/github-cloud/redirect.ts
- app/components/project-settings/general-settings/github-project/index.ts
- app/components/security/analysis-report-btn/index.ts
- app/services/websocket.ts
- tests/integration/components/file-details/summary-test.js
- app/components/file-details/api-scan/captured-apis/index.ts
- app/services/freshdesk.ts
- app/components/security/analysis-details/attachments/index.ts
- app/components/organization-analytics/index.ts
- app/components/security/analysis-list/table/action/index.ts
- app/components/user-registration/via-org-invite/index.ts
- app/components/file-details/manual-scan/request-access/index.ts
- app/components/security/file-details-actions/index.ts
- app/components/system-status/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/services/ajax.ts
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (25)
app/components/account-settings/security/multi-factor-auth/index.ts (2)
18-26: LGTM! Good type definitions.The addition of proper type definitions for
IreneAjaxService,AjaxError, andTokenDataimproves type safety and code clarity.
31-32: LGTM! Improved type safety.Replacing
anywithIreneAjaxServicetype for the ajax service declaration enhances type safety and maintainability.tests/integration/components/partner/client-report-download-test.js (2)
6-12: LGTM! Clean import organization.The imports are well-organized and properly grouped.
707-709: LGTM! Improved test reliability.The addition of
waitUntilhelps ensure that assertions are made only after the expected UI changes have occurred, making the tests more reliable and reducing flakiness.Also applies to: 770-771
app/components/user-login/index.ts (2)
31-31: Service injection properly typed with IreneAjaxService.This change aligns with the broader effort to replace ember-ajax and improve type safety across the codebase.
78-81: API contract change requires test updates.The modified request payload structure needs alignment with existing tests.
app/services/configuration.ts (3)
72-72: LGTM! Successfully migrated to IreneAjaxService.The change aligns with the PR objective of replacing ember-ajax.
Line range hint
93-116: LGTM! Properties are now properly typed.The explicit type annotations for
themeData,imageData, andintegrationDataimprove type safety.
132-134: LGTM! Type-safe AJAX requests.The AJAX requests are now properly typed with their respective response types.
Also applies to: 204-206, 228-230
app/components/user-registration/via-login-page/index.ts (1)
16-17: LGTM! Service migration looks good.The changes correctly migrate from
networkservice toajaxservice, including proper type imports and error handling updates.Also applies to: 39-39, 92-92, 96-96
app/services/ajax.ts (3)
10-41: LGTM! Well-structured type definitions.The interfaces are well-documented and provide clear type safety for request options and headers.
144-144: Type the session service properly.Using
anytype for the session service bypasses TypeScript's type checking.- @service declare session: any; + @service declare session: { + data: { + authenticated: { + b64token?: string; + }; + }; + };
280-280: Replace delete operator with undefined assignment.Using the delete operator can impact performance.
- delete (fetchOptions as RequestOptions).namespace; + (fetchOptions as RequestOptions).namespace = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
tests/integration/components/file-details/vulnerability-analysis-details/edit-analysis-button-test.js (2)
401-403: LGTM! Request body handling updated correctly.The test correctly updates the request body handling from URL-encoded to JSON format.
597-597: LGTM! DELETE request body assertion updated.The test correctly updates the assertion to expect JSON format for the DELETE request body.
app/services/oidc.ts (5)
6-6: LGTM: Import change aligns with PR objective.The import of
IreneAjaxServicetype is correctly added to support the transition from ember-ajax to the new ajax service.
35-35: LGTM: Service type declaration updated correctly.The ajax service declaration is properly typed with
IreneAjaxService, maintaining type safety in the transition.
64-88: Enhance error handling robustness.The error handling needs improvement to be more defensive and handle unexpected error structures.
This is a duplicate of a previous review comment. The suggested improvements include:
- Using
Partial<OidcAuthorizeResult>for safer type assertion- Adding null checks
- Adding a fallback error handler
- Improving error logging
102-128: Improve error handling implementation.The error handling implementation should be enhanced using a proper Error class.
This is a duplicate of a previous review comment suggesting the creation of an
OidcErrorclass for better error handling and stack traces.
156-168:⚠️ Potential issueFix unsafe error handling.
The error handling has several issues:
- Unsafe type assertion of error
- Unsafe property access without null checks
- No URL validation before redirect
- Plain object thrown instead of proper Error class
Apply these improvements:
- const err = error as OidcAuthorizationResponse; + const err = error as Partial<OidcAuthorizationResponse>; - if (err.validation_result.error) { + if (err?.validation_result?.error) { const redirectUrl = err.validation_result.redirect_url; - if (err.validation_result.redirect_url) { + if (redirectUrl && this.isValidRedirectUrl(redirectUrl)) { this.window.location.href = redirectUrl; return; } else { - throw { - name: 'Error', - code: err.validation_result.error?.code, - description: err.validation_result.error?.description, - }; + throw new OidcError( + err.validation_result.error?.code, + err.validation_result.error?.description + ); } } + throw error;Likely invalid or redundant comment.
app/components/project-settings/analysis-settings/vulnerability-list/index.ts (3)
18-19: LGTM! Type-safe imports and well-defined interface.The new imports and VulnerabilityPreferenceResponse interface enhance type safety and provide clear contract for the API response structure.
Also applies to: 32-38
44-44: LGTM! Improved type safety for ajax service.Replacing
anywithIreneAjaxServicetype provides better type checking and aligns with the PR objective of migrating from ember-ajax.
193-198: LGTM! Well-structured ajax request with proper content type.The changes correctly implement:
- Form data encoding using the utility function
- Proper content type header
- Type-safe response handling
tests/integration/components/user-login/recover-password-test.js (2)
62-64: Robust approach for parsing the request body.Using
JSON.parse(req.requestBody)in the test ensures the username is extracted in a controlled, structured manner. Since this is a test environment, full error handling may not be necessary, but be mindful of any scenarios where non-JSON content could be sent. Otherwise, looks good.
106-108: Consistent JSON parsing logic for request body.Repeating the same pattern here maintains consistency and clarity when verifying the request payload. This helps avoid string-splitting issues and matches the real-world handling of JSON request bodies. No changes needed.
app/components/project-settings/analysis-settings/vulnerability-list/index.ts
Outdated
Show resolved
Hide resolved
SmitGala
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
from
bbd717a to
581a7ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/routes/authenticated/github-cloud/redirect.ts (1)
35-35:⚠️ Potential issueEnhance error handling robustness.
The current error handling might break if the error payload structure is unexpected.
- this.notify.error(`Error Occured: ${(err as AjaxError).payload.message}`); + const errorMessage = err instanceof AjaxError + ? err.payload?.message ?? 'Unknown error occurred' + : 'Failed to connect to GitHub'; + this.notify.error(`Error Occurred: ${errorMessage}`);
🧹 Nitpick comments (14)
app/routes/authenticated/github-cloud/redirect.ts (2)
10-13: Consider expanding the GithubResponse type.While the current type definition covers the basic needs, GitHub's API typically returns additional useful fields like
id,avatar_url, andtypethat might be valuable for future use.type GithubResponse = { login: string; html_url: string; + id: number; + avatar_url: string; + type: string; };
29-31: Add token validation.Consider adding validation for the token before making the request to handle empty or invalid tokens gracefully.
+ if (!token) { + this.notify.error('GitHub token is required'); + return this.router.transitionTo('authenticated.dashboard'); + } + const data = await this.ajax.post<GithubResponse>(url, { data: { token }, });app/services/ajax.ts (5)
40-40: Consider using a more specific type forpayload.Using
anytype bypasses TypeScript's type checking. Consider using a more specific type or a generic parameter.- payload: any; + payload: unknown;
26-31: Consider making DefaultHeaders more restrictive.The interface allows any string value for 'X-Product'. Consider using a union type of allowed values.
interface DefaultHeaders { /** Basic authentication token */ Authorization?: string; /** Product identifier header */ - 'X-Product': string; + 'X-Product': 'ak' | 'am' | 'ab'; // Replace with actual allowed values }
95-108: Consider using the URL API instead of DOM manipulation.Using the DOM API for URL parsing is less efficient. The URL API provides a more direct solution.
export function parseURL(str: string) { - const element = document.createElement('a'); - element.href = str; - - return { - href: element.href, - protocol: element.protocol, - hostname: element.hostname, - port: element.port, - pathname: element.pathname, - search: element.search, - hash: element.hash, - }; + const url = new URL(str, window.location.origin); + return { + href: url.href, + protocol: url.protocol, + hostname: url.hostname, + port: url.port, + pathname: url.pathname, + search: url.search, + hash: url.hash, + }; }
262-299: Enhance error type handling in makeRequest.The error response could be better typed to ensure type safety throughout the error handling chain.
async makeRequest<T>(url: string, options: RequestOptions = {}): Promise<T> { + type ErrorResponse = Response & AjaxError; const finalUrl = this._buildURL(url, options); // ... existing code ... if (!response.ok) { const parsedResponse = await this.parseResponseText(response); - const errorResponse = { + const errorResponse: ErrorResponse = { ...response, ...(typeof parsedResponse === 'object' ? parsedResponse : { message: parsedResponse }), payload: parsedResponse, }; throw errorResponse; } return (await this.parseResponseText(response)) as T; }🧰 Tools
🪛 Biome (1.9.4)
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
147-147: Consider using environment type checking.The environment check could be more type-safe using TypeScript's type system.
- host: string = ENV.environment === 'test' ? window.location.origin : ENV.host; + host: string = (ENV.environment as 'test' | 'development' | 'production') === 'test' + ? window.location.origin + : ENV.host;app/components/file/report-drawer/va-reports/report-item/index.ts (2)
7-7: Be cautious about using test waiters in production code.
ImportingwaitForPromisefrom@ember/test-waitersis consistently helpful for tests, but in production, it might impose additional overhead or complexities. Consider using Ember Concurrency tasks directly if you aim to manage concurrency in non-test environments.
94-100: Handle the case whenthis.fileReportis null or undefined more explicitly.
Your current logic only proceeds ifthis.fileReportis defined, but does not notify or handle the scenario wherethis.fileReportis missing. Consider providing user feedback or an early return to clarify why no action is taken in that scenario.app/components/account-settings/security/multi-factor-auth/index.ts (2)
Line range hint
179-184: Add missing type annotations for consistency.While some Ajax calls are properly typed with
TokenData, others are missing type annotations. For consistency and type safety, add type annotations to all Ajax calls.For example:
staInitialEmail = task(async () => { try { - await this.ajax.post(this.mfaEndpoint, { + await this.ajax.post<TokenData>(this.mfaEndpoint, { data: { method: ENUMS.MFA_METHOD.TOTP, }, });Also applies to: 312-317, 458-463, 682-687
Line range hint
198-205: Consider centralizing error handling logic.The error handling pattern is repeated across multiple functions. Consider extracting this into a reusable utility function to reduce code duplication and maintain consistency.
private handleAjaxError(error: AjaxError, showInvalidOTP = true) { const errorObj = error.payload || {}; const otpMsg = errorObj.otp && errorObj.otp[0]; if (otpMsg && showInvalidOTP) { this.notify.error(this.tInvalidOTP); } else { this.notify.error(this.tsomethingWentWrong); } }Also applies to: 341-348, 465-475, 689-699, 722-729, 890-897, 913-920
🧰 Tools
🪛 Biome (1.9.4)
[error] 199-199: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/partner/client-report-download/index.js (1)
137-138: Capture the error context in the catch block for better debugging.Currently, the
catchblock does not capture the error parameter, which may eliminate valuable debugging information. Consider capturing the error parameter to provide more robust error reporting and troubleshooting:} catch { - this.notify.error(this.intl.t('downloadUrlNotFound')); +} catch (error) { + this.notify.error(parseError(error, this.intl.t('downloadUrlNotFound'))); }app/services/configuration.ts (2)
132-134: Consider adding retry logic for configuration fetches.Configuration fetches are critical for application functionality. Consider adding retry logic with exponential backoff.
+ private async retryRequest<T>( + endpoint: string, + maxRetries = 3, + baseDelay = 1000 + ): Promise<T> { + let lastError: unknown; + for (let i = 0; i < maxRetries; i++) { + try { + return await this.ajax.request<T>(endpoint); + } catch (error) { + lastError = error; + if (i < maxRetries - 1) { + await new Promise(resolve => + setTimeout(resolve, baseDelay * Math.pow(2, i)) + ); + } + } + } + throw lastError; + } async frontendConfigFetch() { try { - const data = await this.ajax.request<FrontendConfigResponse>( - this.frontendConfigEndpoint - ); + const data = await this.retryRequest<FrontendConfigResponse>( + this.frontendConfigEndpoint + );
204-206: Apply consistent error handling across fetch methods.The server and dashboard configuration fetches should follow the same pattern with retry logic.
Also applies to: 228-230
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (82)
app/adapters/file-report.ts(1 hunks)app/authenticators/base.ts(2 hunks)app/authenticators/irene.ts(1 hunks)app/authenticators/saml2.ts(1 hunks)app/components/account-settings/developer-settings/personal-token-list/delete-token/index.ts(3 hunks)app/components/account-settings/developer-settings/personal-token-list/index.ts(3 hunks)app/components/account-settings/general/select-language/index.ts(3 hunks)app/components/account-settings/security/multi-factor-auth/index.ts(16 hunks)app/components/account-settings/security/password-change/index.ts(4 hunks)app/components/analysis-risk/override-edit-drawer/override-form/index.ts(0 hunks)app/components/api-filter/index.ts(3 hunks)app/components/app-monitoring/version-table/actions/index.ts(2 hunks)app/components/appknox-wrapper/index.ts(0 hunks)app/components/attachment-detail/index.ts(1 hunks)app/components/dynamicscan-automation-upselling-feature/index.ts(1 hunks)app/components/file-details/api-scan/captured-apis/footer/index.ts(2 hunks)app/components/file-details/api-scan/captured-apis/index.ts(3 hunks)app/components/file-details/dynamic-scan/action/drawer/device-pref-table/index.ts(0 hunks)app/components/file-details/dynamic-scan/action/drawer/index.ts(2 hunks)app/components/file-details/dynamic-scan/action/index.ts(3 hunks)app/components/file-details/dynamic-scan/drawer-old/index.ts(2 hunks)app/components/file-details/dynamic-scan/results/index.ts(0 hunks)app/components/file-details/manual-scan/index.ts(3 hunks)app/components/file-details/manual-scan/request-access/index.ts(2 hunks)app/components/file-details/static-scan/index.ts(3 hunks)app/components/file-details/summary/file-tags/index.ts(2 hunks)app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts(2 hunks)app/components/file-details/vulnerability-analysis/header/index.ts(2 hunks)app/components/file/report-drawer/sbom-reports/sample/index.ts(1 hunks)app/components/file/report-drawer/va-reports/report-item/index.ts(2 hunks)app/components/home-page/index.ts(0 hunks)app/components/organization-analytics/app-scan-chart/index.ts(2 hunks)app/components/organization-analytics/index.ts(2 hunks)app/components/organization-billing/invoice-list/download-action/index.ts(2 hunks)app/components/organization-billing/subscription/index.ts(3 hunks)app/components/organization-invitation-list/invite-delete/index.ts(1 hunks)app/components/organization-invitation-list/invite-resend/index.ts(0 hunks)app/components/organization-team/create-team/index.ts(0 hunks)app/components/partner/client-report-download/index.js(1 hunks)app/components/project-preferences-old/provider/index.ts(2 hunks)app/components/project-preferences/provider/index.ts(2 hunks)app/components/project-settings/analysis-settings/toggle-analysis/index.ts(2 hunks)app/components/project-settings/analysis-settings/vulnerability-list/index.ts(4 hunks)app/components/project-settings/general-settings/dynamicscan-automation-settings/index.ts(2 hunks)app/components/project-settings/general-settings/github-project/index.ts(3 hunks)app/components/project-settings/view-scenario/index.ts(0 hunks)app/components/public-api-docs/index.ts(2 hunks)app/components/security/analysis-details/attachments/index.ts(4 hunks)app/components/security/analysis-details/index.ts(2 hunks)app/components/security/analysis-list/header/index.ts(2 hunks)app/components/security/analysis-list/table/action/index.ts(2 hunks)app/components/security/analysis-report-btn/index.ts(3 hunks)app/components/security/download-app/index.ts(1 hunks)app/components/security/file-details-actions/index.ts(4 hunks)app/components/security/file-search-list/download/index.ts(2 hunks)app/components/security/purge-api-analysis/index.ts(1 hunks)app/components/system-status/index.ts(3 hunks)app/components/user-login/index.ts(3 hunks)app/components/user-login/recover-password/index.ts(3 hunks)app/components/user-login/reset-password/index.ts(3 hunks)app/components/user-registration/via-login-page/index.ts(3 hunks)app/components/user-registration/via-org-invite/index.ts(3 hunks)app/components/user-registration/via-partner-invite/index.ts(4 hunks)app/routes/authenticated/dashboard/organization-settings/index.ts(1 hunks)app/routes/authenticated/github-cloud/redirect.ts(2 hunks)app/routes/authenticated/payment-success.ts(1 hunks)app/routes/invite.ts(1 hunks)app/services/ajax.js(0 hunks)app/services/ajax.ts(1 hunks)app/services/configuration.ts(6 hunks)app/services/devicefarm.ts(2 hunks)app/services/freshdesk.ts(3 hunks)app/services/oidc.ts(5 hunks)app/services/organization.ts(1 hunks)app/services/websocket.ts(3 hunks)package.json(0 hunks)tests/integration/components/file-details/dynamic-scan/manual-test.js(5 hunks)tests/integration/components/file-details/summary-test.js(1 hunks)tests/integration/components/file-details/vulnerability-analysis-details/edit-analysis-button-test.js(2 hunks)tests/integration/components/partner/client-report-download-test.js(1 hunks)tests/integration/components/project-settings/general-settings/dynamicscan-automation-settings/index-test.js(1 hunks)tests/integration/components/security/analysis-details-test.js(2 hunks)
⛔ Files not processed due to max files limit (6)
- tests/integration/components/user-login/recover-password-test.js
- tests/integration/components/via-org-invite-test.js
- tests/integration/components/via-partner-invite-test.js
- tests/unit/services/ajax-test.js
- tsconfig.json
- types/irene/index.d.ts
💤 Files with no reviewable changes (10)
- app/components/analysis-risk/override-edit-drawer/override-form/index.ts
- app/components/project-settings/view-scenario/index.ts
- app/components/organization-team/create-team/index.ts
- app/components/file-details/dynamic-scan/action/drawer/device-pref-table/index.ts
- app/components/organization-invitation-list/invite-resend/index.ts
- app/components/appknox-wrapper/index.ts
- app/services/ajax.js
- package.json
- app/components/home-page/index.ts
- app/components/file-details/dynamic-scan/results/index.ts
🚧 Files skipped from review as they are similar to previous changes (62)
- app/components/organization-analytics/index.ts
- tests/integration/components/file-details/summary-test.js
- app/services/organization.ts
- app/components/security/file-search-list/download/index.ts
- app/components/security/analysis-list/table/action/index.ts
- app/components/project-settings/general-settings/dynamicscan-automation-settings/index.ts
- app/services/websocket.ts
- app/components/project-settings/analysis-settings/toggle-analysis/index.ts
- tests/integration/components/project-settings/general-settings/dynamicscan-automation-settings/index-test.js
- app/components/user-login/recover-password/index.ts
- tests/integration/components/partner/client-report-download-test.js
- app/components/file-details/api-scan/captured-apis/footer/index.ts
- app/components/project-preferences/provider/index.ts
- app/authenticators/irene.ts
- app/components/project-preferences-old/provider/index.ts
- app/authenticators/saml2.ts
- app/components/organization-billing/subscription/index.ts
- app/services/freshdesk.ts
- app/components/file-details/manual-scan/index.ts
- app/components/security/analysis-details/index.ts
- app/components/api-filter/index.ts
- app/components/file-details/dynamic-scan/action/drawer/index.ts
- app/components/dynamicscan-automation-upselling-feature/index.ts
- app/components/security/file-details-actions/index.ts
- app/components/user-registration/via-org-invite/index.ts
- app/components/file-details/dynamic-scan/action/index.ts
- app/components/system-status/index.ts
- app/services/devicefarm.ts
- app/components/public-api-docs/index.ts
- app/authenticators/base.ts
- app/components/user-registration/via-partner-invite/index.ts
- app/components/attachment-detail/index.ts
- app/components/security/analysis-list/header/index.ts
- app/components/account-settings/general/select-language/index.ts
- app/components/security/analysis-details/attachments/index.ts
- app/components/file-details/vulnerability-analysis/header/index.ts
- app/components/file-details/api-scan/captured-apis/index.ts
- app/components/security/download-app/index.ts
- app/components/file/report-drawer/sbom-reports/sample/index.ts
- app/components/organization-invitation-list/invite-delete/index.ts
- app/components/user-registration/via-login-page/index.ts
- app/components/organization-analytics/app-scan-chart/index.ts
- app/components/security/purge-api-analysis/index.ts
- app/components/organization-billing/invoice-list/download-action/index.ts
- app/routes/authenticated/payment-success.ts
- app/routes/invite.ts
- app/components/user-login/reset-password/index.ts
- app/components/file-details/dynamic-scan/drawer-old/index.ts
- app/components/app-monitoring/version-table/actions/index.ts
- app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts
- app/components/security/analysis-report-btn/index.ts
- app/adapters/file-report.ts
- app/components/file-details/manual-scan/request-access/index.ts
- app/components/file-details/summary/file-tags/index.ts
- tests/integration/components/security/analysis-details-test.js
- app/components/account-settings/developer-settings/personal-token-list/index.ts
- app/components/file-details/static-scan/index.ts
- app/components/project-settings/general-settings/github-project/index.ts
- app/components/account-settings/developer-settings/personal-token-list/delete-token/index.ts
- app/routes/authenticated/dashboard/organization-settings/index.ts
- app/components/account-settings/security/password-change/index.ts
- app/components/user-login/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/services/ajax.ts
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (21)
app/routes/authenticated/github-cloud/redirect.ts (2)
7-8: LGTM! Clean type imports.The type imports are well-structured and properly use the
typekeyword for better tree-shaking.
16-16: LGTM! Improved type safety.Good job replacing the
anytype withIreneAjaxServicefor better type checking.tests/integration/components/file-details/vulnerability-analysis-details/edit-analysis-button-test.js (1)
401-403: LGTM! Consistent JSON request body handling.The change from URL-encoded to JSON request bodies is consistent with modern API practices and aligns with the changes across the codebase.
Also applies to: 597-597
tests/integration/components/file-details/dynamic-scan/manual-test.js (1)
109-111: LGTM! Consistent JSON request body handling and enum usage.The changes maintain consistency in:
- Using JSON for request bodies
- Direct comparison with enum values instead of string conversion
Also applies to: 1013-1013, 1599-1627
app/services/ajax.ts (2)
144-144: Type the session service properly.Using
anytype for the session service bypasses TypeScript's type checking.
280-280: Replace delete operator with undefined assignment.Using the delete operator can impact performance.
🧰 Tools
🪛 Biome (1.9.4)
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
app/components/project-settings/analysis-settings/vulnerability-list/index.ts (4)
18-19: LGTM! Well-structured type definitions.The new imports and interface definition align well with the PR objective of replacing ember-ajax. The VulnerabilityPreferenceResponse interface provides good type safety for the ajax response.
Also applies to: 32-38
44-44: LGTM! Enhanced type safety for ajax service.Good improvement replacing
anywith the specificIreneAjaxServicetype.
182-182: Consider using consistent type for risk parameter.While the form data handling looks good, consider using only
numbertype for the risk parameter to maintain consistency across the codebase, as mentioned in previous reviews.- async (data: { risk: number | string; comment: string }) => { + async (data: { risk: number; comment: string }) => {Also applies to: 193-198
174-175: Verify server-side handling of empty strings.The change from
nullto empty string values forriskandcommentmight affect server-side processing. Please ensure that the backend API properly handles empty strings for these fields.✅ Verification successful
Empty string values are compatible with API contract
The change from
nullto empty strings is safe as verified by the interface definitions:
- The
saveVulnerabilityPreferencestask explicitly acceptsstringtype for both fields- The
VulnerabilityPreferenceResponseinterface shows these fields are properly handled by the API🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for API endpoint handling of risk and comment fields rg -t ts "risk.*=.*null" -B2 -A2 rg -t ts "comment.*=.*null" -B2 -A2Length of output: 1520
Script:
#!/bin/bash # Search for vulnerability-related API endpoints and interfaces ast-grep --pattern 'interface $_ { $$$ risk: $_ $$$ }' # Search for API calls related to vulnerability settings rg -t ts "updateVulnerability|saveVulnerability|vulnerability.*settings" -B2 -A2Length of output: 2215
app/components/account-settings/security/multi-factor-auth/index.ts (3)
18-19: LGTM! Type definitions enhance code safety.The addition of proper type definitions for the Ajax service and token data improves type safety and makes the code more maintainable.
Also applies to: 23-26
527-527: Consider removing or masking tokens from debug logs.Logging tokens, even in debug mode, can be a security concern. Consider masking or removing sensitive data from logs.
- debug('SwitchTOEmail: App OTP Token Data ' + tokenData?.token); + debug('SwitchTOEmail: App OTP Token Data [MASKED]'); - debug('sta: Email OTP Token Data ' + tokenData.token); + debug('sta: Email OTP Token Data [MASKED]');Also applies to: 764-764
458-463: Verify rate limiting for MFA operations.The code handles sensitive MFA operations. Ensure that rate limiting is implemented at the API level to prevent brute force attacks.
Also applies to: 682-687
app/services/oidc.ts (6)
6-6: LGTM! Clean import of IreneAjaxService.The type import aligns with the PR objective of transitioning from ember-ajax to ember-fetch.
35-35: LGTM! Clean service declaration.The transition from NetworkService to IreneAjaxService is properly typed.
64-88: Enhance error handling robustness.The current error handling makes assumptions about the error structure that might not always hold true.
102-128: Improve error object implementation.Consider using a proper Error class for better error handling and stack traces.
79-80:⚠️ Potential issueAdd URL validation before redirects.
Direct URL redirects without validation could pose a security risk.
Add URL validation before performing redirects:
+ private isValidRedirectUrl(url: string): boolean { + try { + const redirectUrl = new URL(url); + const allowedDomains = ['your-domain.com']; // Configure your allowed domains + return allowedDomains.some(domain => redirectUrl.hostname.endsWith(domain)); + } catch { + return false; + } + } if (err.redirect_url) { + if (this.isValidRedirectUrl(err.redirect_url)) { this.window.location.href = err.redirect_url; + } else { + this.notify.error(this.intl.t('invalidRedirectUrl')); + } }Likely invalid or redundant comment.
146-168: 🛠️ Refactor suggestionApply consistent error handling pattern.
Apply the same error handling improvements as suggested for other methods:
- Use proper Error classes
- Add URL validation
- Implement defensive type casting
- const err = error as OidcAuthorizationResponse; + const err = error as Partial<OidcAuthorizationResponse>; - if (err.validation_result.error) { + if (err?.validation_result?.error) { if (err.validation_result.redirect_url) { + if (this.isValidRedirectUrl(err.validation_result.redirect_url)) { this.window.location.href = err.validation_result.redirect_url; + } else { + this.notify.error(this.intl.t('invalidRedirectUrl')); + } return; } else { - throw { - name: 'Error', - code: err.validation_result.error?.code, - description: err.validation_result.error?.description, - }; + throw new OidcError( + err.validation_result.error?.code, + err.validation_result.error?.description + ); } } + throw error;Likely invalid or redundant comment.
app/services/configuration.ts (2)
20-69: LGTM! Well-structured type definitions.The type definitions are comprehensive and follow TypeScript best practices, providing clear interfaces for configuration data.
72-72: LGTM! Clean service declaration.The transition from NetworkService to IreneAjaxService is properly typed.
SmitGala
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
from
581a7ae to
1c0291c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
app/components/account-settings/security/multi-factor-auth/index.ts (1)
527-527:⚠️ Potential issueRemove sensitive token data from debug logs.
Logging tokens, even in debug mode, poses a security risk. Consider masking the token value.
- debug('SwitchTOEmail: App OTP Token Data ' + tokenData?.token); + debug('SwitchTOEmail: App OTP Token Data [MASKED]');app/routes/authenticated/github-cloud/redirect.ts (1)
35-35:⚠️ Potential issueEnhance error handling robustness.
The current error handling might break if the error payload structure is unexpected.
Apply this diff to make the error handling more robust:
- this.notify.error(`Error Occured: ${(err as AjaxError).payload.message}`); + const errorMessage = err instanceof AjaxError + ? err.payload?.message ?? 'Unknown error occurred' + : 'Failed to connect to GitHub'; + this.notify.error(`Error Occurred: ${errorMessage}`);Also, note the typo: "Occured" should be "Occurred".
app/services/ajax.ts (1)
144-144: 🛠️ Refactor suggestionType the session service properly.
Using
anytype for the session service bypasses TypeScript's type checking.Apply this diff:
- @service declare session: any; + @service declare session: { + data: { + authenticated: { + b64token?: string; + }; + }; + };
🧹 Nitpick comments (6)
app/services/configuration.ts (2)
75-75: Consider adding proper type for session service.The
sessionservice is currently typed asany. Consider adding a proper type definition to maintain type safety throughout the service.- @service declare session: any; + @service declare session: { + isAuthenticated: boolean; + // Add other required properties + };
Line range hint
171-177: Consider adding protection against race conditions.The current promise caching pattern could lead to race conditions if multiple calls occur simultaneously before the first promise is assigned. Consider using an atomic check-and-set pattern.
private async getConfigWithCache<T>( promiseKey: 'frontendPromise' | 'serverPromise' | 'dashboardPromise', fetchMethod: () => Promise<void>, resultGetter: () => T ): Promise<T> { // Atomic check and set if (!this[promiseKey]) { const promise = fetchMethod(); this[promiseKey] = promise; await promise; } else { await this[promiseKey]; } return resultGetter(); } // Usage example: async getFrontendConfig() { return this.getConfigWithCache( 'frontendPromise', () => this.frontendConfigFetch(), () => this.frontendData ); }Also applies to: 218-224, 241-247
app/services/ajax.ts (1)
262-299: Consider adding request timeout handling.The
makeRequestmethod should handle request timeouts to prevent hanging requests.Consider adding a timeout option:
async makeRequest<T>(url: string, options: RequestOptions = {}): Promise<T> { const finalUrl = this._buildURL(url, options); + const timeout = options.timeout ?? 30000; // Default 30s timeout + + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), timeout); + const fetchOptions: RequestInit = { ...options, headers, body: this.processRequestBody(options.data), + signal: controller.signal, }; - const response = await fetch(finalUrl, fetchOptions); + try { + const response = await fetch(finalUrl, fetchOptions); + clearTimeout(timeoutId); // ... rest of the code + } catch (error) { + clearTimeout(timeoutId); + throw error; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
app/components/security/analysis-details/attachments/index.ts (1)
110-116: Consider improving endpoint types and reducing duplication
The type assertions (
as string) on endpoints suggest thatENV.endpointsmight need better typing. Consider defining proper types for the endpoints object to avoid these assertions.The namespace 'api/hudson-api' is duplicated. Consider extracting it to a constant or configuration value.
+ const API_NAMESPACE = 'api/hudson-api'; + uploadFile = task(async (file: UploadFile) => { // ... const fileData = await this.ajax.post<UploadAttachmentResponse>( ENV.endpoints['uploadFile'] as string, { - namespace: 'api/hudson-api', + namespace: API_NAMESPACE, data, } ); // ... await this.ajax.post(ENV.endpoints['uploadedAttachment'] as string, { - namespace: 'api/hudson-api', + namespace: API_NAMESPACE, data: fileDetailsData, });Also applies to: 131-134
app/components/security/analysis-report-btn/index.ts (2)
22-32: LGTM! Consider using an enum for report formats.The type definitions are well-structured and provide good type safety. The use of
keyofinDownloadReportTypeensures format values align with available response keys.Consider using an enum for report formats to centralize these values:
export enum ReportFormat { Excel = 'xlsx', JapaneseHTML = 'html_ja', EnglishHTML = 'html_en' } type DownloadReportsResponse = { [K in ReportFormat]: string; }; type DownloadReportType = { label: string; format: ReportFormat; icon: string; };
144-144: Consider using a more specific error type.The type annotations improve type safety, but we could make it even better.
Consider adding type information for error handling:
import type { AjaxError } from 'irene/services/ajax'; try { // ... existing code ... } catch (error: unknown) { if (error instanceof AjaxError) { this.notify.error(parseError(error, this.intl.t('pleaseTryAgain'))); } else { throw error; // Re-throw unexpected errors } }Also applies to: 150-152
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (82)
app/adapters/file-report.ts(1 hunks)app/authenticators/base.ts(2 hunks)app/authenticators/irene.ts(1 hunks)app/authenticators/saml2.ts(1 hunks)app/components/account-settings/developer-settings/personal-token-list/delete-token/index.ts(3 hunks)app/components/account-settings/developer-settings/personal-token-list/index.ts(3 hunks)app/components/account-settings/general/select-language/index.ts(3 hunks)app/components/account-settings/security/multi-factor-auth/index.ts(16 hunks)app/components/account-settings/security/password-change/index.ts(4 hunks)app/components/analysis-risk/override-edit-drawer/override-form/index.ts(0 hunks)app/components/api-filter/index.ts(3 hunks)app/components/app-monitoring/version-table/actions/index.ts(2 hunks)app/components/appknox-wrapper/index.ts(0 hunks)app/components/attachment-detail/index.ts(1 hunks)app/components/dynamicscan-automation-upselling-feature/index.ts(1 hunks)app/components/file-details/api-scan/captured-apis/footer/index.ts(2 hunks)app/components/file-details/api-scan/captured-apis/index.ts(3 hunks)app/components/file-details/dynamic-scan/action/drawer/device-pref-table/index.ts(0 hunks)app/components/file-details/dynamic-scan/action/drawer/index.ts(2 hunks)app/components/file-details/dynamic-scan/action/index.ts(3 hunks)app/components/file-details/dynamic-scan/drawer-old/index.ts(2 hunks)app/components/file-details/dynamic-scan/results/index.ts(0 hunks)app/components/file-details/manual-scan/index.ts(3 hunks)app/components/file-details/manual-scan/request-access/index.ts(2 hunks)app/components/file-details/static-scan/index.ts(3 hunks)app/components/file-details/summary/file-tags/index.ts(2 hunks)app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts(2 hunks)app/components/file-details/vulnerability-analysis/header/index.ts(2 hunks)app/components/file/report-drawer/sbom-reports/sample/index.ts(1 hunks)app/components/file/report-drawer/va-reports/report-item/index.ts(2 hunks)app/components/home-page/index.ts(0 hunks)app/components/organization-analytics/app-scan-chart/index.ts(2 hunks)app/components/organization-analytics/index.ts(2 hunks)app/components/organization-billing/invoice-list/download-action/index.ts(2 hunks)app/components/organization-billing/subscription/index.ts(3 hunks)app/components/organization-invitation-list/invite-delete/index.ts(1 hunks)app/components/organization-invitation-list/invite-resend/index.ts(0 hunks)app/components/organization-team/create-team/index.ts(0 hunks)app/components/partner/client-report-download/index.js(1 hunks)app/components/project-preferences-old/provider/index.ts(2 hunks)app/components/project-preferences/provider/index.ts(2 hunks)app/components/project-settings/analysis-settings/toggle-analysis/index.ts(2 hunks)app/components/project-settings/analysis-settings/vulnerability-list/index.ts(4 hunks)app/components/project-settings/general-settings/dynamicscan-automation-settings/index.ts(2 hunks)app/components/project-settings/general-settings/github-project/index.ts(3 hunks)app/components/project-settings/view-scenario/index.ts(0 hunks)app/components/public-api-docs/index.ts(2 hunks)app/components/security/analysis-details/attachments/index.ts(4 hunks)app/components/security/analysis-details/index.ts(2 hunks)app/components/security/analysis-list/header/index.ts(2 hunks)app/components/security/analysis-list/table/action/index.ts(2 hunks)app/components/security/analysis-report-btn/index.ts(3 hunks)app/components/security/download-app/index.ts(1 hunks)app/components/security/file-details-actions/index.ts(4 hunks)app/components/security/file-search-list/download/index.ts(2 hunks)app/components/security/purge-api-analysis/index.ts(1 hunks)app/components/system-status/index.ts(3 hunks)app/components/user-login/index.ts(3 hunks)app/components/user-login/recover-password/index.ts(3 hunks)app/components/user-login/reset-password/index.ts(3 hunks)app/components/user-registration/via-login-page/index.ts(3 hunks)app/components/user-registration/via-org-invite/index.ts(3 hunks)app/components/user-registration/via-partner-invite/index.ts(4 hunks)app/routes/authenticated/dashboard/organization-settings/index.ts(1 hunks)app/routes/authenticated/github-cloud/redirect.ts(2 hunks)app/routes/authenticated/payment-success.ts(1 hunks)app/routes/invite.ts(1 hunks)app/services/ajax.js(0 hunks)app/services/ajax.ts(1 hunks)app/services/configuration.ts(6 hunks)app/services/devicefarm.ts(2 hunks)app/services/freshdesk.ts(3 hunks)app/services/oidc.ts(5 hunks)app/services/organization.ts(1 hunks)app/services/websocket.ts(3 hunks)package.json(0 hunks)tests/integration/components/file-details/dynamic-scan/manual-test.js(5 hunks)tests/integration/components/file-details/summary-test.js(1 hunks)tests/integration/components/file-details/vulnerability-analysis-details/edit-analysis-button-test.js(2 hunks)tests/integration/components/partner/client-report-download-test.js(1 hunks)tests/integration/components/project-settings/general-settings/dynamicscan-automation-settings/index-test.js(1 hunks)tests/integration/components/security/analysis-details-test.js(1 hunks)
⛔ Files not processed due to max files limit (6)
- tests/integration/components/user-login/recover-password-test.js
- tests/integration/components/via-org-invite-test.js
- tests/integration/components/via-partner-invite-test.js
- tests/unit/services/ajax-test.js
- tsconfig.json
- types/irene/index.d.ts
💤 Files with no reviewable changes (10)
- app/components/home-page/index.ts
- package.json
- app/components/file-details/dynamic-scan/results/index.ts
- app/components/project-settings/view-scenario/index.ts
- app/components/organization-team/create-team/index.ts
- app/components/organization-invitation-list/invite-resend/index.ts
- app/components/analysis-risk/override-edit-drawer/override-form/index.ts
- app/components/appknox-wrapper/index.ts
- app/components/file-details/dynamic-scan/action/drawer/device-pref-table/index.ts
- app/services/ajax.js
🚧 Files skipped from review as they are similar to previous changes (65)
- app/components/organization-invitation-list/invite-delete/index.ts
- app/authenticators/irene.ts
- app/routes/invite.ts
- app/authenticators/base.ts
- app/components/project-preferences/provider/index.ts
- app/services/websocket.ts
- app/components/app-monitoring/version-table/actions/index.ts
- app/components/security/purge-api-analysis/index.ts
- app/components/security/file-search-list/download/index.ts
- app/components/security/analysis-list/table/action/index.ts
- app/routes/authenticated/payment-success.ts
- app/components/organization-analytics/app-scan-chart/index.ts
- tests/integration/components/partner/client-report-download-test.js
- app/components/dynamicscan-automation-upselling-feature/index.ts
- app/components/file-details/vulnerability-analysis/header/index.ts
- app/components/project-settings/analysis-settings/toggle-analysis/index.ts
- app/components/file-details/manual-scan/request-access/index.ts
- app/components/file/report-drawer/va-reports/report-item/index.ts
- app/authenticators/saml2.ts
- app/components/file-details/dynamic-scan/action/drawer/index.ts
- tests/integration/components/file-details/vulnerability-analysis-details/edit-analysis-button-test.js
- app/components/project-preferences-old/provider/index.ts
- app/components/api-filter/index.ts
- app/services/organization.ts
- app/components/project-settings/general-settings/github-project/index.ts
- tests/integration/components/security/analysis-details-test.js
- app/components/user-login/recover-password/index.ts
- app/components/file-details/dynamic-scan/action/index.ts
- app/components/user-registration/via-org-invite/index.ts
- app/services/devicefarm.ts
- app/components/security/analysis-details/index.ts
- app/components/security/analysis-list/header/index.ts
- app/components/file/report-drawer/sbom-reports/sample/index.ts
- app/components/file-details/summary/file-tags/index.ts
- app/components/file-details/api-scan/captured-apis/footer/index.ts
- app/components/user-registration/via-login-page/index.ts
- app/components/partner/client-report-download/index.js
- app/components/system-status/index.ts
- app/components/file-details/manual-scan/index.ts
- app/components/project-settings/general-settings/dynamicscan-automation-settings/index.ts
- app/components/account-settings/general/select-language/index.ts
- app/services/oidc.ts
- app/services/freshdesk.ts
- app/components/security/download-app/index.ts
- app/components/user-login/index.ts
- app/components/organization-analytics/index.ts
- tests/integration/components/file-details/dynamic-scan/manual-test.js
- app/components/organization-billing/invoice-list/download-action/index.ts
- tests/integration/components/file-details/summary-test.js
- app/components/file-details/dynamic-scan/drawer-old/index.ts
- app/components/security/file-details-actions/index.ts
- app/components/account-settings/security/password-change/index.ts
- app/components/user-registration/via-partner-invite/index.ts
- app/components/account-settings/developer-settings/personal-token-list/index.ts
- tests/integration/components/project-settings/general-settings/dynamicscan-automation-settings/index-test.js
- app/adapters/file-report.ts
- app/routes/authenticated/dashboard/organization-settings/index.ts
- app/components/account-settings/developer-settings/personal-token-list/delete-token/index.ts
- app/components/file-details/static-scan/index.ts
- app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts
- app/components/public-api-docs/index.ts
- app/components/attachment-detail/index.ts
- app/components/organization-billing/subscription/index.ts
- app/components/user-login/reset-password/index.ts
- app/components/file-details/api-scan/captured-apis/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/services/ajax.ts
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (24)
app/components/account-settings/security/multi-factor-auth/index.ts (4)
18-26: LGTM! Type definitions improve code safety.The addition of proper type definitions for the Ajax service and token data enhances type safety and makes the code more maintainable.
32-32: LGTM! Service type properly defined.Replacing
anywithIreneAjaxServicetype improves type safety and aligns with the PR objective.
Line range hint
743-757: LGTM! Improved type safety for token handling.The explicit type declaration and proper type casting for token data improve type safety and prevent potential runtime errors.
🧰 Tools
🪛 Biome (1.9.4)
[error] 759-759: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
198-204: LGTM! Consistent error handling with proper typing.The error handling has been consistently updated across all locations to use the
AjaxErrortype, improving type safety and making the code more maintainable.Also applies to: 465-474, 491-499, 661-669, 689-699, 722-730, 890-897, 913-921
🧰 Tools
🪛 Biome (1.9.4)
[error] 199-199: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/services/configuration.ts (3)
20-70: Well-structured type definitions!The newly added TypeScript interfaces are comprehensive, properly structured, and provide excellent type safety for the configuration data. The naming conventions are consistent and clear.
72-72: LGTM! Service dependency updated correctly.The change from NetworkService to IreneAjaxService aligns with the PR objective of transitioning to ember-fetch.
132-134: LGTM! Network requests properly typed.The ajax requests are well-implemented with proper type parameters and consistent error handling.
Also applies to: 204-206, 228-230
app/routes/authenticated/github-cloud/redirect.ts (3)
10-13: LGTM! Well-structured type definition.The
GithubResponsetype properly defines the expected GitHub API response shape, enhancing type safety.
16-16: LGTM! Good type safety improvement.Updating the ajax service type from
anytoIreneAjaxServicealigns with the PR objective and improves type safety.
29-31: LGTM! Properly typed API call.The post request is correctly typed with
GithubResponse, ensuring type safety for the response data.app/services/ajax.ts (3)
10-41: LGTM! Well-structured type definitions.The interfaces
RequestOptions,DefaultHeaders, andAjaxErrorare well-defined and provide good type safety for the service.
280-280: Replace delete operator with undefined assignment.Using the delete operator can impact performance.
Apply this diff:
- delete (fetchOptions as RequestOptions).namespace; + (fetchOptions as RequestOptions).namespace = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
308-322: LGTM! Robust request body processing.The
processRequestBodymethod handles different types of request data appropriately, including FormData, Blob, and JSON serialization.app/components/project-settings/analysis-settings/vulnerability-list/index.ts (5)
18-19: LGTM! Proper typing for ajax serviceThe imports align with the PR objective of replacing ember-ajax with proper TypeScript types.
32-38: LGTM! Well-defined response interfaceThe
VulnerabilityPreferenceResponseinterface properly defines the structure of the API response, improving type safety.
44-44: LGTM! Properly typed ajax serviceReplacing
anywithIreneAjaxServiceimproves type safety and aligns with the PR objective.
Line range hint
182-198: Consider using consistent types for risk parameterThe union type
number | stringfor risk parameter was previously flagged. Consider using justnumbertype to maintain consistency across the codebase.The rest of the changes look good:
- Proper use of
buildURLEncodedFormData- Correct content-type header
- Well-typed response
174-175: Verify API contract for reset operationThe change from
nullto empty strings for reset operation needs verification.Let's verify the API contract for resetting vulnerability preferences:
✅ Verification successful
Empty strings are valid for resetting vulnerability preferences
The change from
nullto empty strings is correct and matches the API contract:
- The
saveVulnerabilityPreferencestask explicitly acceptsstring | numbertype for risk- The data is properly form-encoded before being sent to the API
- The reset operation is designed to work with empty strings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for reset-related API calls and their handling rg -t ts "risk.*=.*[\"']" -g "app/**/*vulnerability*.ts" -B2 -A2Length of output: 68
Script:
#!/bin/bash # Search for vulnerability preference types and interfaces ast-grep --pattern 'interface $_VulnerabilityPreference { $$$ }' # Search for reset-related functions and API calls rg -t ts "(reset|clear).*vulnerability" -B2 -A2 # Search for API endpoints related to vulnerability preferences rg -t ts "(put|post|patch).*vulnerability.*preference" -B2 -A2Length of output: 183
Script:
#!/bin/bash # Look at the content of the file where changes are made cat "app/components/project-settings/analysis-settings/vulnerability-list/index.ts" # Search for preference-related patterns more broadly rg -t ts "preference.*risk|risk.*preference" -B2 -A2 # Search for type definitions with risk field ast-grep --pattern 'type $_ = { risk$_ $$$ }'Length of output: 7688
app/components/security/analysis-details/attachments/index.ts (4)
14-14: LGTM! Import of IreneAjaxService typeThe addition of the IreneAjaxService type import improves type safety by replacing the generic
anytype.
23-28: LGTM! Well-structured type definitionThe UploadAttachmentResponse type clearly defines the expected structure of the upload response, enhancing type safety and code clarity.
35-35: LGTM! Type-safe service declarationThe ajax service declaration now uses the specific IreneAjaxService type instead of any, improving type safety.
35-35: Verify ember-fetch implementationWhile the type updates look good, we should verify that the IreneAjaxService implementation uses ember-fetch internally to align with the PR objective of removing ember-ajax.
Also applies to: 110-116, 131-134
✅ Verification successful
Migration to ember-fetch verified successfully
The IreneAjaxService has been properly migrated to use ember-fetch. The implementation uses the native
fetchAPI for all HTTP requests, and there are no remaining ember-ajax dependencies. The reference in buildurl.ts is just a comment documenting the source of copied utility functions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if IreneAjaxService uses ember-fetch rg -l "ember-fetch" "app/services/ajax.ts" # Verify ember-ajax removal rg -l "ember-ajax"Length of output: 84
Script:
#!/bin/bash # Check IreneAjaxService implementation ast-grep --pattern 'class IreneAjaxService { $$$ }' # Check buildurl.ts content cat app/services/buildurl.ts # Additional check for any ember-ajax imports rg "from ['|\"]ember-ajax" -A 2Length of output: 2779
Script:
#!/bin/bash # Try to view ajax service implementation with a different pattern cat app/services/ajax.ts # Double check for any remaining ember-ajax dependencies rg -l "@ember-ajax" package.jsonLength of output: 10593
app/components/security/analysis-report-btn/index.ts (2)
38-38: LGTM! Type safety improvement.Good improvement replacing
anywithIreneAjaxServicetype, which aligns with the PR objective of migrating from ember-ajax.
Line range hint
150-157: Consider adding response validation.The response handling could be more robust with runtime validation.
Let's verify if there's a validation utility in use elsewhere in the codebase:
Consider adding runtime validation:
function isValidReportResponse(data: unknown): data is DownloadReportsResponse { return ( typeof data === 'object' && data !== null && ('xlsx' in data || 'html_en' in data || 'html_ja' in data) && Object.values(data).every((url) => typeof url === 'string') ); } const data = await this.ajax.request<unknown>(url, { namespace: 'api/hudson-api', }); if (!isValidReportResponse(data)) { throw new Error('Invalid response format'); }
SmitGala
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
from
1c0291c to
048a75b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
app/components/account-settings/security/multi-factor-auth/index.ts (1)
Line range hint
743-757: Type safety improvement needed for tokenData initialization.The
tokenDatavariable is declared with typeTokenDatabut might be undefined when used. Consider adding null checks or providing a default value.Apply this diff to improve type safety:
- let tokenData: TokenData; + let tokenData: TokenData | undefined; await this.staInitialEmail.perform(); do { debug('sta: In Email OTP Loop'); const emailOTPData = await this.staConfirmEmailOTP.perform(); if (emailOTPData.cancel) { return; } - tokenData = (await this.staVerifyEmailOTP.perform( - emailOTPData.otp - )) as TokenData; + const response = await this.staVerifyEmailOTP.perform(emailOTPData.otp); + if (!response) { + continue; + } + tokenData = response;🧰 Tools
🪛 Biome (1.9.4)
[error] 759-759: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
♻️ Duplicate comments (3)
app/components/account-settings/security/multi-factor-auth/index.ts (1)
527-527:⚠️ Potential issueSecurity concern: Token data exposed in debug logs.
Logging sensitive data like tokens, even in debug mode, poses a security risk as logs might be exposed.
Apply this diff to mask the sensitive data:
- debug('SwitchTOEmail: App OTP Token Data ' + tokenData?.token); + debug('SwitchTOEmail: App OTP Token Data [MASKED]');app/services/oidc.ts (2)
65-89: 🛠️ Refactor suggestionEnhance error handling robustness in authorizeOidcAppPermissions.
The error handling could be more defensive and should validate URLs before redirecting.
Apply this diff to improve error handling:
try { const data = await this.ajax.post<OidcAuthorizeResult>( this.oidcAuthorizeEndpoint, { data: { oidc_token: token, allow }, } ); - if (data.valid && data.redirect_url) { + if (data?.valid && data?.redirect_url && this.isValidRedirectUrl(data.redirect_url)) { this.window.location.href = data.redirect_url; } } catch (e) { const err = e as AjaxError; - if (err.payload.error) { + if (err?.payload?.error) { - if (err.payload.redirect_url) { + if (err.payload?.redirect_url && this.isValidRedirectUrl(err.payload.redirect_url)) { this.window.location.href = err.payload.redirect_url; } else { this.notify.error( - err.payload.error.description || this.intl.t('somethingWentWrong') + err.payload.error?.description || this.intl.t('somethingWentWrong') ); } return; } + this.notify.error(this.intl.t('somethingWentWrong')); + console.error('Unexpected error during OIDC authorization:', e); }
103-130: 🛠️ Refactor suggestionRefactor error handling to use proper Error class.
Both
validateOidcTokenandfetchOidcAuthorizationDatacreate plain error objects. Use a proper Error class for better error handling and stack traces.Add this class at the beginning of the file:
class OidcError extends Error { constructor( public statusCode: number, public code: string | undefined, public description: string | undefined ) { super(description || 'OIDC Validation Error'); this.name = 'OidcError'; } }Then update the error throwing in both methods:
- throw { - name: 'Error', - statusCode: err.status, - code: err.payload.error?.code, - description: err.payload.error?.description, - }; + throw new OidcError( + err.status, + err.payload.error?.code, + err.payload.error?.description + );Also applies to: 148-172
🧹 Nitpick comments (6)
app/services/ajax.ts (1)
33-41: Consider improving the AjaxError interface type safety.The
payloadproperty is typed asany. Consider making it more specific based on your error response structure, or useunknownif the structure is truly unpredictable.- payload: any; + payload: unknown;app/services/configuration.ts (4)
20-69: Consider improving type definitions for better type safety.While the new types greatly improve type safety, consider these enhancements:
- The
enterprisefield inServerResponsecould be more specific thanstring | boolean- Consider using string literal types for the theme
schemefield if there are predefined valuesExample improvement:
type ServerResponse = { // ... enterprise: boolean | 'true' | 'false'; // More specific than string | boolean // ... }; type ThemeData = { scheme: 'light' | 'dark' | 'auto'; // If these are the possible values // ... };
72-72: Type the session service properly instead of using 'any'.The session service is currently typed as
any. Consider creating proper type definitions for better type safety.import SessionService from 'ember-simple-auth/services/session'; // ... @service declare session: SessionService;
Line range hint
136-139: Use strict equality for boolean comparisons.Replace loose equality (
==) with strict equality (===) for boolean comparisons:-this.frontendData.hide_poweredby_logo = data.hide_poweredby_logo == true; +this.frontendData.hide_poweredby_logo = data.hide_poweredby_logo === true; -this.frontendData.registration_enabled ||= data.registration_enabled == true; +this.frontendData.registration_enabled ||= data.registration_enabled === true;
Line range hint
171-195: Consider refactoring similar configuration fetch methods.The
getFrontendConfig,getServerConfig, andgetDashboardConfigmethods share similar patterns. Consider refactoring to reduce code duplication.type ConfigType = 'frontend' | 'server' | 'dashboard'; private getConfigData(type: ConfigType) { const configs = { frontend: { promise: this.frontendPromise, fetch: () => this.frontendConfigFetch(), data: this.frontendData, }, server: { promise: this.serverPromise, fetch: () => this.serverConfigFetch(), data: this.serverData, }, dashboard: { promise: this.dashboardPromise, fetch: () => this.dashboardConfigFetch(), data: this.dashboardData, requiresAuth: true, }, }; const config = configs[type]; if (!config.promise && (!config.requiresAuth || this.session.isAuthenticated)) { config.promise = config.fetch(); } return config.data; }Also applies to: 216-226, 238-248
app/services/oidc.ts (1)
103-130: Extract common error handling logic.The error handling logic is duplicated across methods. Consider extracting it into a shared private method.
Add this private method:
private handleOidcError(error: AjaxError, isAuthorizationError = false) { const errorData = isAuthorizationError ? error.payload?.validation_result : error.payload; if (error.status === 400 || errorData?.error) { if (errorData?.redirect_url && this.isValidRedirectUrl(errorData.redirect_url)) { this.window.location.href = errorData.redirect_url; return; } throw new OidcError( error.status, errorData?.error?.code, errorData?.error?.description ); } throw error; }Then update the catch blocks to use this method:
} catch (error) { const err = error as AjaxError; - if (err.status === 400 || err.payload.error) { - // ... existing error handling ... - } + this.handleOidcError(err); }Also applies to: 148-172
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (82)
app/adapters/file-report.ts(1 hunks)app/authenticators/base.ts(2 hunks)app/authenticators/irene.ts(1 hunks)app/authenticators/saml2.ts(1 hunks)app/components/account-settings/developer-settings/personal-token-list/delete-token/index.ts(3 hunks)app/components/account-settings/developer-settings/personal-token-list/index.ts(3 hunks)app/components/account-settings/general/select-language/index.ts(3 hunks)app/components/account-settings/security/multi-factor-auth/index.ts(16 hunks)app/components/account-settings/security/password-change/index.ts(4 hunks)app/components/analysis-risk/override-edit-drawer/override-form/index.ts(0 hunks)app/components/api-filter/index.ts(3 hunks)app/components/app-monitoring/version-table/actions/index.ts(2 hunks)app/components/appknox-wrapper/index.ts(0 hunks)app/components/attachment-detail/index.ts(1 hunks)app/components/dynamicscan-automation-upselling-feature/index.ts(1 hunks)app/components/file-details/api-scan/captured-apis/footer/index.ts(2 hunks)app/components/file-details/api-scan/captured-apis/index.ts(3 hunks)app/components/file-details/dynamic-scan/action/drawer/device-pref-table/index.ts(0 hunks)app/components/file-details/dynamic-scan/action/drawer/index.ts(2 hunks)app/components/file-details/dynamic-scan/action/index.ts(3 hunks)app/components/file-details/dynamic-scan/drawer-old/index.ts(2 hunks)app/components/file-details/dynamic-scan/results/index.ts(0 hunks)app/components/file-details/manual-scan/index.ts(3 hunks)app/components/file-details/manual-scan/request-access/index.ts(2 hunks)app/components/file-details/static-scan/index.ts(3 hunks)app/components/file-details/summary/file-tags/index.ts(2 hunks)app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts(2 hunks)app/components/file-details/vulnerability-analysis/header/index.ts(2 hunks)app/components/file/report-drawer/sbom-reports/sample/index.ts(1 hunks)app/components/file/report-drawer/va-reports/report-item/index.ts(2 hunks)app/components/home-page/index.ts(0 hunks)app/components/organization-analytics/app-scan-chart/index.ts(2 hunks)app/components/organization-analytics/index.ts(2 hunks)app/components/organization-billing/invoice-list/download-action/index.ts(2 hunks)app/components/organization-billing/subscription/index.ts(3 hunks)app/components/organization-invitation-list/invite-delete/index.ts(1 hunks)app/components/organization-invitation-list/invite-resend/index.ts(0 hunks)app/components/organization-team/create-team/index.ts(0 hunks)app/components/partner/client-report-download/index.js(1 hunks)app/components/project-preferences-old/provider/index.ts(2 hunks)app/components/project-preferences/provider/index.ts(2 hunks)app/components/project-settings/analysis-settings/toggle-analysis/index.ts(2 hunks)app/components/project-settings/analysis-settings/vulnerability-list/index.ts(3 hunks)app/components/project-settings/general-settings/dynamicscan-automation-settings/index.ts(2 hunks)app/components/project-settings/general-settings/github-project/index.ts(3 hunks)app/components/project-settings/view-scenario/index.ts(0 hunks)app/components/public-api-docs/index.ts(2 hunks)app/components/security/analysis-details/attachments/index.ts(4 hunks)app/components/security/analysis-details/index.ts(2 hunks)app/components/security/analysis-list/header/index.ts(2 hunks)app/components/security/analysis-list/table/action/index.ts(2 hunks)app/components/security/analysis-report-btn/index.ts(3 hunks)app/components/security/download-app/index.ts(1 hunks)app/components/security/file-details-actions/index.ts(4 hunks)app/components/security/file-search-list/download/index.ts(2 hunks)app/components/security/purge-api-analysis/index.ts(1 hunks)app/components/system-status/index.ts(3 hunks)app/components/user-login/index.ts(3 hunks)app/components/user-login/recover-password/index.ts(3 hunks)app/components/user-login/reset-password/index.ts(3 hunks)app/components/user-registration/via-login-page/index.ts(3 hunks)app/components/user-registration/via-org-invite/index.ts(3 hunks)app/components/user-registration/via-partner-invite/index.ts(4 hunks)app/routes/authenticated/dashboard/organization-settings/index.ts(1 hunks)app/routes/authenticated/github-cloud/redirect.ts(2 hunks)app/routes/authenticated/payment-success.ts(1 hunks)app/routes/invite.ts(1 hunks)app/services/ajax.js(0 hunks)app/services/ajax.ts(1 hunks)app/services/configuration.ts(6 hunks)app/services/devicefarm.ts(2 hunks)app/services/freshdesk.ts(3 hunks)app/services/oidc.ts(5 hunks)app/services/organization.ts(1 hunks)app/services/websocket.ts(3 hunks)package.json(0 hunks)tests/integration/components/file-details/dynamic-scan/manual-test.js(5 hunks)tests/integration/components/file-details/summary-test.js(1 hunks)tests/integration/components/file-details/vulnerability-analysis-details/edit-analysis-button-test.js(2 hunks)tests/integration/components/partner/client-report-download-test.js(1 hunks)tests/integration/components/project-settings/general-settings/dynamicscan-automation-settings/index-test.js(1 hunks)tests/integration/components/security/analysis-details-test.js(1 hunks)
⛔ Files not processed due to max files limit (6)
- tests/integration/components/user-login/recover-password-test.js
- tests/integration/components/via-org-invite-test.js
- tests/integration/components/via-partner-invite-test.js
- tests/unit/services/ajax-test.js
- tsconfig.json
- types/irene/index.d.ts
💤 Files with no reviewable changes (10)
- app/components/project-settings/view-scenario/index.ts
- package.json
- app/components/file-details/dynamic-scan/results/index.ts
- app/components/analysis-risk/override-edit-drawer/override-form/index.ts
- app/components/file-details/dynamic-scan/action/drawer/device-pref-table/index.ts
- app/components/appknox-wrapper/index.ts
- app/components/home-page/index.ts
- app/components/organization-team/create-team/index.ts
- app/components/organization-invitation-list/invite-resend/index.ts
- app/services/ajax.js
🚧 Files skipped from review as they are similar to previous changes (67)
- app/components/organization-invitation-list/invite-delete/index.ts
- tests/integration/components/project-settings/general-settings/dynamicscan-automation-settings/index-test.js
- app/components/security/download-app/index.ts
- app/routes/authenticated/payment-success.ts
- app/components/security/file-search-list/download/index.ts
- app/authenticators/irene.ts
- app/routes/invite.ts
- app/authenticators/base.ts
- app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts
- app/components/security/analysis-list/table/action/index.ts
- app/components/file/report-drawer/va-reports/report-item/index.ts
- tests/integration/components/partner/client-report-download-test.js
- app/components/organization-analytics/index.ts
- app/components/app-monitoring/version-table/actions/index.ts
- app/components/project-settings/general-settings/dynamicscan-automation-settings/index.ts
- app/components/partner/client-report-download/index.js
- tests/integration/components/file-details/summary-test.js
- app/components/security/purge-api-analysis/index.ts
- app/services/organization.ts
- app/components/project-preferences/provider/index.ts
- app/services/websocket.ts
- app/components/organization-analytics/app-scan-chart/index.ts
- app/adapters/file-report.ts
- app/components/security/analysis-details/attachments/index.ts
- app/components/organization-billing/invoice-list/download-action/index.ts
- app/components/file-details/static-scan/index.ts
- app/services/devicefarm.ts
- app/components/project-settings/general-settings/github-project/index.ts
- app/components/organization-billing/subscription/index.ts
- app/authenticators/saml2.ts
- app/components/file-details/dynamic-scan/action/drawer/index.ts
- app/components/file-details/dynamic-scan/action/index.ts
- app/components/security/analysis-list/header/index.ts
- app/components/project-settings/analysis-settings/toggle-analysis/index.ts
- app/components/user-login/recover-password/index.ts
- app/components/file-details/summary/file-tags/index.ts
- app/components/file-details/api-scan/captured-apis/footer/index.ts
- app/components/system-status/index.ts
- app/components/security/file-details-actions/index.ts
- app/components/file-details/api-scan/captured-apis/index.ts
- app/components/security/analysis-report-btn/index.ts
- app/components/file-details/manual-scan/index.ts
- app/routes/authenticated/github-cloud/redirect.ts
- app/components/user-login/index.ts
- app/services/freshdesk.ts
- tests/integration/components/file-details/dynamic-scan/manual-test.js
- app/components/file-details/vulnerability-analysis/header/index.ts
- app/components/security/analysis-details/index.ts
- app/routes/authenticated/dashboard/organization-settings/index.ts
- app/components/user-registration/via-org-invite/index.ts
- app/components/public-api-docs/index.ts
- app/components/project-preferences-old/provider/index.ts
- app/components/account-settings/security/password-change/index.ts
- app/components/user-registration/via-login-page/index.ts
- app/components/file/report-drawer/sbom-reports/sample/index.ts
- app/components/api-filter/index.ts
- app/components/user-registration/via-partner-invite/index.ts
- app/components/account-settings/general/select-language/index.ts
- app/components/account-settings/developer-settings/personal-token-list/delete-token/index.ts
- app/components/account-settings/developer-settings/personal-token-list/index.ts
- app/components/user-login/reset-password/index.ts
- app/components/file-details/dynamic-scan/drawer-old/index.ts
- app/components/dynamicscan-automation-upselling-feature/index.ts
- app/components/file-details/manual-scan/request-access/index.ts
- app/components/attachment-detail/index.ts
- tests/integration/components/file-details/vulnerability-analysis-details/edit-analysis-button-test.js
- tests/integration/components/security/analysis-details-test.js
🧰 Additional context used
🪛 Biome (1.9.4)
app/services/ajax.ts
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (15)
app/components/account-settings/security/multi-factor-auth/index.ts (4)
23-26: LGTM! Well-defined type for token data.The
TokenDatatype definition improves type safety for handling authentication tokens and secrets.
32-32: LGTM! Improved type safety for ajax service.The ajax service is now properly typed with
IreneAjaxServiceinstead ofany.
Line range hint
179-183: LGTM! Consistent type-safe ajax responses.The ajax POST requests are properly typed with
TokenDatareturn type, ensuring type safety across the codebase.Also applies to: 312-316, 458-463, 682-687
Line range hint
198-204: LGTM! Improved error handling with proper typing.Error handling is now properly typed with
AjaxError, providing better type safety and consistent error handling across the component.Also applies to: 341-347, 465-474, 491-497, 661-669, 689-699, 722-728, 890-897, 913-919
🧰 Tools
🪛 Biome (1.9.4)
[error] 199-199: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/project-settings/analysis-settings/vulnerability-list/index.ts (3)
Line range hint
18-38: LGTM! Well-structured type definitions.The
VulnerabilityPreferenceResponseinterface properly defines the response structure with appropriate types for all fields.
44-44: LGTM! Type-safe service injection.The ajax service is now properly typed with
IreneAjaxService, improving type safety.
193-198: LGTM! Proper form data handling and type safety.The implementation correctly:
- Uses the new
buildURLEncodedFormDatautility- Sets the appropriate content type for form-encoded data
- Specifies the response type for improved type safety
app/services/ajax.ts (4)
144-144: Type the session service properly.Using
anytype for the session service bypasses TypeScript's type checking.
280-280: Replace delete operator with undefined assignment.Using the delete operator can impact performance.
🧰 Tools
🪛 Biome (1.9.4)
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
346-378: LGTM! Well-implemented HTTP methods.The HTTP methods are properly typed with generics and correctly override the request method in options.
284-296: LGTM! Robust error handling implementation.The error handling:
- Properly parses error responses
- Maintains the original response properties
- Handles both object and string error messages
app/services/configuration.ts (2)
6-6: LGTM! Import change aligns with PR objectives.The import of
IreneAjaxServiceis consistent with the PR's goal of replacing ember-ajax with ember-fetch.
132-134: LGTM! Well-typed AJAX requests with proper error handling.The AJAX requests are properly typed and include good error handling through the logger service.
Also applies to: 204-206, 228-230
app/services/oidc.ts (2)
6-7: LGTM! Import statements are properly typed.The addition of typed imports for
IreneAjaxServiceandAjaxErroraligns with the migration from NetworkService.
36-36: LGTM! Service injection is properly typed.The
ajaxservice is correctly injected with proper TypeScript type declaration.
SmitGala
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
from
048a75b to
f988921
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
app/services/configuration.ts (1)
Consider using
??=for URL and API key configurationsThe current
||=usage could override valid empty string configurations. Consider using??=for:
- URL configurations (favicon, logos, registration_link, etc.)
- Integration API keys (csb_key, hotjar_key, etc.)
Example:
this.imageData.favicon ??= data.images.favicon; this.integrationData.hotjar_key ??= data.integrations.hotjar_key;Keep
||=for theme colors and boolean properties where falsy values aren't valid configurations.🔗 Analysis chain
Line range hint
142-160: Review nullish assignment operator usage.The extensive use of
||=might mask legitimate falsy values (empty strings, false). Consider using??=for properties where falsy values are valid configurations:// Example: If an empty string is a valid logo URL this.imageData.logo_on_darkbg ??= data.images.logo_on_darkbg;Run this script to analyze the impact:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential falsy value assignments in configuration rg -A 1 '\|\|=' app/services/configuration.ts | grep -v 'undefined|null'Length of output: 1782
♻️ Duplicate comments (2)
app/routes/authenticated/github-cloud/redirect.ts (1)
35-35:⚠️ Potential issueEnhance error handling robustness.
The current error handling might break if the error payload structure is unexpected.
Consider this more robust approach:
- this.notify.error(`Error Occured: ${(err as AjaxError).payload.message}`); + const errorMessage = err instanceof AjaxError + ? err.payload?.message ?? 'Unknown error occurred' + : 'Failed to connect to GitHub'; + this.notify.error(`Error Occurred: ${errorMessage}`);Also, note the typo: "Occured" should be "Occurred".
app/services/ajax.ts (1)
144-144: 🛠️ Refactor suggestionType the session service properly.
Using
anytype for the session service bypasses TypeScript's type checking.Consider this typed approach:
- @service declare session: any; + @service declare session: { + data: { + authenticated: { + b64token?: string; + }; + }; + };
🧹 Nitpick comments (6)
app/components/security/analysis-report-btn/index.ts (1)
Line range hint
144-163: Consider security and maintainability improvementsA few suggestions to enhance the code:
- Use URL.join or a dedicated URL builder for more maintainable URL construction
- Add security attributes when opening new windows
Consider this improvement:
- const url = [ENV.endpoints['reports'], this.fileId, 'download_url'].join( - '/' - ); + const url = new URL( + `${ENV.endpoints['reports']}/${this.fileId}/download_url`, + window.location.origin + ).toString(); const data = await this.ajax.request<DownloadReportsResponse>(url, { namespace: 'api/hudson-api', }); if (data && data[type.format]) { - this.window.open(data[type.format], '_blank'); + this.window.open(data[type.format], '_blank', 'noopener,noreferrer'); } else {app/components/account-settings/security/multi-factor-auth/index.ts (1)
Line range hint
198-205: Consider extracting common error handling logic.The error handling pattern is repeated across multiple methods. Consider extracting it into a utility function to improve maintainability and reduce code duplication.
// Create a utility function private handleAjaxError(error: AjaxError, customErrorHandler?: (errorObj: any) => void) { const errorObj = error.payload || {}; const otpMsg = errorObj.otp && errorObj.otp[0]; if (otpMsg) { this.notify.error(this.tInvalidOTP); if (customErrorHandler) { customErrorHandler(errorObj); } return; } this.notify.error(this.tsomethingWentWrong); } // Usage example try { // ajax call } catch (error) { this.handleAjaxError(error as AjaxError); }Also applies to: 341-348, 465-474, 661-669, 689-699, 722-731, 890-897, 913-920
🧰 Tools
🪛 Biome (1.9.4)
[error] 199-199: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/services/ajax.ts (2)
262-299: Consider adding request timeout handling.The
makeRequestmethod should handle request timeouts to prevent hanging requests.Consider adding a timeout option:
async makeRequest<T>(url: string, options: RequestOptions = {}): Promise<T> { const finalUrl = this._buildURL(url, options); + const timeout = options.timeout ?? 30000; // Default 30s timeout + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), timeout); + + try { const headers = this._shouldSendHeaders(finalUrl, options.host) ? { 'Content-Type': options.contentType || 'application/json', ...this.defaultHeaders, ...options.headers, } : options.headers || {}; const fetchOptions: RequestInit = { ...options, headers, body: this.processRequestBody(options.data), + signal: controller.signal, }; (fetchOptions as RequestOptions).namespace = undefined; const response = await fetch(finalUrl, fetchOptions); + clearTimeout(timeoutId); if (!response.ok) { // ... error handling ... } return (await this.parseResponseText(response)) as T; + } finally { + clearTimeout(timeoutId); + } }🧰 Tools
🪛 Biome (1.9.4)
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
330-338: Consider adding response size limit.The
parseResponseTextmethod should protect against extremely large responses that could cause memory issues.Consider adding a size limit check:
private async parseResponseText<T>(response: Response): Promise<T | string> { + const MAX_RESPONSE_SIZE = 50 * 1024 * 1024; // 50MB limit + const contentLength = parseInt(response.headers.get('content-length') ?? '0'); + + if (contentLength > MAX_RESPONSE_SIZE) { + throw new Error('Response too large'); + } + const text = await response.text(); try { return text ? JSON.parse(text) : {}; } catch { return text; } }app/services/configuration.ts (2)
20-69: Well-structured type definitions!The type definitions are comprehensive and improve type safety. Consider adding JSDoc comments to document the purpose of each type, especially for complex structures like
IntegrationData.Example documentation:
/** * Represents the integration configuration data for various third-party services. * @property freshchat_key - API key for Freshchat integration * @property hotjar_key - API key for Hotjar analytics * ... */ type IntegrationData = { // ... existing properties };
Line range hint
137-141: Consider stricter boolean comparisons.Replace loose equality (
==) with strict equality (===) for boolean comparisons:- this.frontendData.hide_poweredby_logo = data.hide_poweredby_logo == true; + this.frontendData.hide_poweredby_logo = data.hide_poweredby_logo === true; - this.frontendData.registration_enabled ||= data.registration_enabled == true; + this.frontendData.registration_enabled ||= data.registration_enabled === true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (82)
app/adapters/file-report.ts(1 hunks)app/authenticators/base.ts(2 hunks)app/authenticators/irene.ts(1 hunks)app/authenticators/saml2.ts(1 hunks)app/components/account-settings/developer-settings/personal-token-list/delete-token/index.ts(3 hunks)app/components/account-settings/developer-settings/personal-token-list/index.ts(3 hunks)app/components/account-settings/general/select-language/index.ts(3 hunks)app/components/account-settings/security/multi-factor-auth/index.ts(16 hunks)app/components/account-settings/security/password-change/index.ts(4 hunks)app/components/analysis-risk/override-edit-drawer/override-form/index.ts(0 hunks)app/components/api-filter/index.ts(3 hunks)app/components/app-monitoring/version-table/actions/index.ts(2 hunks)app/components/appknox-wrapper/index.ts(0 hunks)app/components/attachment-detail/index.ts(1 hunks)app/components/dynamicscan-automation-upselling-feature/index.ts(1 hunks)app/components/file-details/api-scan/captured-apis/footer/index.ts(2 hunks)app/components/file-details/api-scan/captured-apis/index.ts(3 hunks)app/components/file-details/dynamic-scan/action/drawer/device-pref-table/index.ts(0 hunks)app/components/file-details/dynamic-scan/action/drawer/index.ts(2 hunks)app/components/file-details/dynamic-scan/action/index.ts(3 hunks)app/components/file-details/dynamic-scan/drawer-old/index.ts(2 hunks)app/components/file-details/dynamic-scan/results/index.ts(0 hunks)app/components/file-details/manual-scan/index.ts(3 hunks)app/components/file-details/manual-scan/request-access/index.ts(2 hunks)app/components/file-details/static-scan/index.ts(3 hunks)app/components/file-details/summary/file-tags/index.ts(2 hunks)app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts(2 hunks)app/components/file-details/vulnerability-analysis/header/index.ts(2 hunks)app/components/file/report-drawer/sbom-reports/sample/index.ts(1 hunks)app/components/file/report-drawer/va-reports/report-item/index.ts(2 hunks)app/components/home-page/index.ts(0 hunks)app/components/organization-analytics/app-scan-chart/index.ts(2 hunks)app/components/organization-analytics/index.ts(2 hunks)app/components/organization-billing/invoice-list/download-action/index.ts(2 hunks)app/components/organization-billing/subscription/index.ts(3 hunks)app/components/organization-invitation-list/invite-delete/index.ts(1 hunks)app/components/organization-invitation-list/invite-resend/index.ts(0 hunks)app/components/organization-team/create-team/index.ts(0 hunks)app/components/partner/client-report-download/index.js(1 hunks)app/components/project-preferences-old/provider/index.ts(2 hunks)app/components/project-preferences/provider/index.ts(2 hunks)app/components/project-settings/analysis-settings/toggle-analysis/index.ts(2 hunks)app/components/project-settings/analysis-settings/vulnerability-list/index.ts(3 hunks)app/components/project-settings/general-settings/dynamicscan-automation-settings/index.ts(2 hunks)app/components/project-settings/general-settings/github-project/index.ts(3 hunks)app/components/project-settings/view-scenario/index.ts(0 hunks)app/components/public-api-docs/index.ts(2 hunks)app/components/security/analysis-details/attachments/index.ts(4 hunks)app/components/security/analysis-details/index.ts(2 hunks)app/components/security/analysis-list/header/index.ts(2 hunks)app/components/security/analysis-list/table/action/index.ts(2 hunks)app/components/security/analysis-report-btn/index.ts(3 hunks)app/components/security/download-app/index.ts(1 hunks)app/components/security/file-details-actions/index.ts(4 hunks)app/components/security/file-search-list/download/index.ts(2 hunks)app/components/security/purge-api-analysis/index.ts(1 hunks)app/components/system-status/index.ts(3 hunks)app/components/user-login/index.ts(3 hunks)app/components/user-login/recover-password/index.ts(3 hunks)app/components/user-login/reset-password/index.ts(3 hunks)app/components/user-registration/via-login-page/index.ts(3 hunks)app/components/user-registration/via-org-invite/index.ts(3 hunks)app/components/user-registration/via-partner-invite/index.ts(4 hunks)app/routes/authenticated/dashboard/organization-settings/index.ts(1 hunks)app/routes/authenticated/github-cloud/redirect.ts(2 hunks)app/routes/authenticated/payment-success.ts(1 hunks)app/routes/invite.ts(1 hunks)app/services/ajax.js(0 hunks)app/services/ajax.ts(1 hunks)app/services/configuration.ts(6 hunks)app/services/devicefarm.ts(2 hunks)app/services/freshdesk.ts(3 hunks)app/services/network.ts(0 hunks)app/services/oidc.ts(5 hunks)app/services/organization.ts(1 hunks)app/services/websocket.ts(3 hunks)package.json(0 hunks)tests/integration/components/file-details/dynamic-scan/manual-test.js(5 hunks)tests/integration/components/file-details/summary-test.js(1 hunks)tests/integration/components/file-details/vulnerability-analysis-details/edit-analysis-button-test.js(2 hunks)tests/integration/components/partner/client-report-download-test.js(1 hunks)tests/integration/components/project-settings/general-settings/dynamicscan-automation-settings/index-test.js(1 hunks)
⛔ Files not processed due to max files limit (7)
- tests/integration/components/security/analysis-details-test.js
- tests/integration/components/user-login/recover-password-test.js
- tests/integration/components/via-org-invite-test.js
- tests/integration/components/via-partner-invite-test.js
- tests/unit/services/ajax-test.js
- tsconfig.json
- types/irene/index.d.ts
💤 Files with no reviewable changes (11)
- app/components/file-details/dynamic-scan/results/index.ts
- app/components/project-settings/view-scenario/index.ts
- app/components/home-page/index.ts
- package.json
- app/components/organization-team/create-team/index.ts
- app/components/appknox-wrapper/index.ts
- app/services/network.ts
- app/components/analysis-risk/override-edit-drawer/override-form/index.ts
- app/services/ajax.js
- app/components/file-details/dynamic-scan/action/drawer/device-pref-table/index.ts
- app/components/organization-invitation-list/invite-resend/index.ts
🚧 Files skipped from review as they are similar to previous changes (65)
- app/components/organization-invitation-list/invite-delete/index.ts
- tests/integration/components/partner/client-report-download-test.js
- app/components/security/download-app/index.ts
- app/authenticators/irene.ts
- app/authenticators/base.ts
- tests/integration/components/project-settings/general-settings/dynamicscan-automation-settings/index-test.js
- app/routes/authenticated/payment-success.ts
- app/authenticators/saml2.ts
- app/components/project-preferences/provider/index.ts
- app/services/organization.ts
- app/components/project-settings/general-settings/dynamicscan-automation-settings/index.ts
- app/components/file-details/manual-scan/index.ts
- app/components/security/file-search-list/download/index.ts
- app/components/organization-analytics/app-scan-chart/index.ts
- app/components/project-preferences-old/provider/index.ts
- app/components/dynamicscan-automation-upselling-feature/index.ts
- app/components/file-details/dynamic-scan/drawer-old/index.ts
- app/components/file-details/api-scan/captured-apis/footer/index.ts
- tests/integration/components/file-details/summary-test.js
- app/components/organization-analytics/index.ts
- app/components/security/analysis-list/table/action/index.ts
- app/components/project-settings/analysis-settings/toggle-analysis/index.ts
- app/components/file-details/manual-scan/request-access/index.ts
- app/services/websocket.ts
- app/adapters/file-report.ts
- app/components/file-details/summary/file-tags/index.ts
- app/components/file/report-drawer/va-reports/report-item/index.ts
- app/components/partner/client-report-download/index.js
- app/components/organization-billing/subscription/index.ts
- app/components/security/analysis-details/attachments/index.ts
- app/components/security/analysis-list/header/index.ts
- app/components/file-details/dynamic-scan/action/drawer/index.ts
- app/components/attachment-detail/index.ts
- app/routes/invite.ts
- app/components/organization-billing/invoice-list/download-action/index.ts
- app/services/freshdesk.ts
- app/components/security/file-details-actions/index.ts
- app/components/file-details/static-scan/index.ts
- app/components/project-settings/general-settings/github-project/index.ts
- app/components/user-registration/via-org-invite/index.ts
- app/components/app-monitoring/version-table/actions/index.ts
- app/services/devicefarm.ts
- app/components/file-details/vulnerability-analysis/header/index.ts
- app/components/security/purge-api-analysis/index.ts
- app/components/user-login/index.ts
- app/components/api-filter/index.ts
- app/routes/authenticated/dashboard/organization-settings/index.ts
- app/components/file-details/dynamic-scan/action/index.ts
- app/components/file/report-drawer/sbom-reports/sample/index.ts
- app/components/system-status/index.ts
- tests/integration/components/file-details/vulnerability-analysis-details/edit-analysis-button-test.js
- app/components/user-login/recover-password/index.ts
- app/components/public-api-docs/index.ts
- app/components/account-settings/developer-settings/personal-token-list/delete-token/index.ts
- app/services/oidc.ts
- app/components/security/analysis-details/index.ts
- app/components/account-settings/security/password-change/index.ts
- app/components/user-registration/via-login-page/index.ts
- app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts
- app/components/file-details/api-scan/captured-apis/index.ts
- app/components/user-login/reset-password/index.ts
- app/components/account-settings/developer-settings/personal-token-list/index.ts
- app/components/user-registration/via-partner-invite/index.ts
- app/components/account-settings/general/select-language/index.ts
- app/components/project-settings/analysis-settings/vulnerability-list/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/services/ajax.ts
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (19)
tests/integration/components/file-details/dynamic-scan/manual-test.js (4)
Line range hint
109-113: LGTM! Improved request body parsing.The change from URL-encoded string parsing to JSON parsing aligns with the migration to ember-fetch and provides a more robust way to handle request bodies in tests.
1013-1013: LGTM! Correct enum usage for device type verification.The assertion correctly uses the ENUMS.DEVICE_TYPE.PHONE_REQUIRED constant for device type verification.
Line range hint
1599-1617: LGTM! Comprehensive device preference reset verification.The test properly verifies that device preferences (both type and platform version) are correctly reset after the dynamic scan enters an in-progress state.
1627-1627: LGTM! Correct enum usage for device type assertion.The assertion correctly uses the ENUMS.DEVICE_TYPE.PHONE_REQUIRED constant for verifying device type changes.
app/components/security/analysis-report-btn/index.ts (3)
13-13: Well-structured type definitions!The new types provide excellent type safety:
DownloadReportsResponseclearly defines the expected API response structureDownloadReportTypeuseskeyofto ensure type safety between the format and response keysAlso applies to: 22-32
38-38: Good type safety improvement!Replacing
anywithIreneAjaxServicetype provides better type checking and aligns with the PR objective of migrating from ember-ajax.
144-144: Excellent type safety improvements!The addition of proper type annotations for both the task parameter and ajax request response enhances type checking and code reliability.
Also applies to: 150-150
app/components/account-settings/security/multi-factor-auth/index.ts (4)
18-26: LGTM! Type definitions improve type safety.The addition of proper type definitions for the Ajax service and token data enhances type safety and code maintainability.
31-32: LGTM! Service declaration is properly typed.The ajax service is now properly typed with
IreneAjaxServiceinstead ofany, which aligns with the PR objective of improving type safety.
Line range hint
179-183: LGTM! Consistent type annotations for ajax responses.The addition of
TokenDatatype annotations to ajax responses improves type safety and ensures consistent handling of token data across the component.Also applies to: 312-316, 458-463, 682-687
527-527: Consider removing or masking tokens from debug logs.Logging tokens, even in debug mode, can be a security concern. Consider masking or removing sensitive data from logs.
- debug('SwitchTOEmail: App OTP Token Data ' + tokenData?.token); + debug('SwitchTOEmail: App OTP Token Data [MASKED]');app/routes/authenticated/github-cloud/redirect.ts (3)
10-13: LGTM! Well-structured type definition.The
GithubResponsetype properly defines the expected response shape from the GitHub API.
16-16: LGTM! Improved type safety.The ajax service is now properly typed with
IreneAjaxServiceinstead ofany.
29-31: LGTM! Type-safe API request.The POST request is now type-safe with the
GithubResponsegeneric type parameter.app/services/ajax.ts (2)
10-41: LGTM! Well-structured type definitions.The interfaces are well-documented and provide comprehensive type safety:
RequestOptionsextendsRequestInitwith additional propertiesDefaultHeadersproperly types the authentication and product headersAjaxErrorprovides a clear error structure
280-280: Replace delete operator with undefined assignment.Using the delete operator can impact performance.
Consider this more performant approach:
- delete (fetchOptions as RequestOptions).namespace; + (fetchOptions as RequestOptions).namespace = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
app/services/configuration.ts (3)
6-6: LGTM! Service dependency updated correctly.The transition from NetworkService to IreneAjaxService aligns with the PR objective of replacing ember-ajax with ember-fetch.
Also applies to: 72-72
Line range hint
93-116: LGTM! Properties properly typed and initialized.Properties are well-structured with appropriate type definitions and safe default values.
132-134: LGTM! Type-safe AJAX requests.The AJAX requests are properly typed using the new IreneAjaxService with appropriate type parameters.
Also applies to: 204-206, 228-230
SmitGala
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
from
f988921 to
7612a4f
Compare
app/components/project-settings/analysis-settings/vulnerability-list/index.ts
Outdated
Show resolved
Hide resolved
app/components/project-settings/analysis-settings/vulnerability-list/index.ts
Outdated
Show resolved
Hide resolved
SmitGala
force-pushed
the
PD-1583-Remove-Ember-Ajax
branch
from
7612a4f to
fd8c42c
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
app/services/configuration.ts (1)
Add validation to prevent usage of unconfigured values
The codebase confirms that configuration values are initialized with empty strings and lack validation. This could lead to runtime issues when these values are used in UI components or integrations. Consider implementing validation in the following locations:
- In the
frontendConfigFetchmethod before assigning values- In the getter methods (
getThemeConfig,getImageConfig,getIntegrationConfig)- Add type guards to ensure required values are present before usage
Example implementation:
private validateThemeData(data: ThemeData): void { if (!data.scheme || !data.primary_color) { throw new Error('Required theme configuration is missing'); } } async getThemeConfig() { if (!this.frontendPromise) { this.frontendPromise = this.frontendConfigFetch(); await this.frontendPromise; } this.validateThemeData(this.themeData); return this.themeData; }🔗 Analysis chain
Line range hint
93-116: Consider adding validation for configuration data.While the type annotations are correct, the properties are initialized with empty strings. Consider adding validation to ensure these required configuration values are present and valid before use.
Example validation check:
private validateThemeData(data: ThemeData): void { if (!data.primary_color || !data.scheme) { throw new Error('Required theme configuration is missing'); } }🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for validation methods and error handling related to configuration rg -A 5 "validate.*(?:Theme|Image|Integration)Data|(?:Theme|Image|Integration)Data.*validate" --type typescript # Look for specific error handling around these configuration objects rg "throw.*(?:configuration|config).*(?:missing|invalid|error)" --type typescript # Search for class methods that might contain validation ast-grep --pattern 'class $_ { $$$ validate$_($_) { $$$ } $$$ }'Length of output: 346
Script:
#!/bin/bash # Find all TypeScript files that might contain validation fd -e ts -x rg -l "validate" # Search for validation and error handling patterns rg "validate|throw.*Error.*config" # Look for usage of these specific data types rg "ThemeData|ImageData|IntegrationData" -A 3 # Search for the configuration class implementation fd "configuration.ts" -x cat {}Length of output: 26117
🧹 Nitpick comments (9)
tests/integration/components/file-details/dynamic-scan/manual-test.js (1)
Line range hint
1599-1613: Consider using a test helper for localStorage interactions.While the verification logic is thorough, direct localStorage access in tests could be abstracted into a test helper for better maintainability and reusability.
+ // Create a test helper (e.g., in test-utils.js) + export function getStoredDevicePreference() { + const windowService = this.owner.lookup('service:browser/window'); + return JSON.parse(windowService.localStorage.getItem('actualDevicePrefData')); + } - const windowService = this.owner.lookup('service:browser/window'); - const actualDevicePrefData = JSON.parse( - windowService.localStorage.getItem('actualDevicePrefData') - ); + const actualDevicePrefData = getStoredDevicePreference();app/components/account-settings/security/multi-factor-auth/index.ts (2)
Line range hint
743-757: Improve type assertion safety.The type assertion on line 757 could be unsafe. Consider using type guards or runtime checks to ensure type safety.
- tokenData = (await this.staVerifyEmailOTP.perform( - emailOTPData.otp - )) as TokenData; + const response = await this.staVerifyEmailOTP.perform(emailOTPData.otp); + if (!response || !('token' in response) || !('secret' in response)) { + throw new Error('Invalid token data received'); + } + tokenData = response;🧰 Tools
🪛 Biome (1.9.4)
[error] 759-759: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
458-474: Standardize error handling pattern.The error handling pattern is repeated across multiple functions. Consider extracting this into a reusable utility function.
type ErrorResponse = { payload?: { otp?: string[]; }; }; function handleMFAError(error: AjaxError, notify: NotificationService) { const errorObj = error.payload || {}; const otpMsg = errorObj.otp?.[0]; if (otpMsg) { notify.error('Invalid OTP'); return true; } notify.error('Something went wrong'); return false; }Then use it in your error handlers:
- const errorObj = (error as AjaxError).payload || {}; - const otpMsg = errorObj.otp && errorObj.otp[0]; - if (otpMsg) { - this.notify.error(this.tInvalidOTP); - return; - } - this.notify.error(this.tsomethingWentWrong); + handleMFAError(error as AjaxError, this.notify);Also applies to: 682-700, 913-922
🧰 Tools
🪛 Biome (1.9.4)
[error] 466-466: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/services/freshdesk.ts (3)
109-111: Consider enhancing error handlingWhile the error handling is functional, consider adding specific error types and user-friendly error messages for better debugging and user experience.
try { const widgetAuthData = await this.ajax.post<WidgetAuthResponse>( this.WIDGET_AUTH_ENDPOINT ); // ... -} catch (error) { - this.logger.error(error); +} catch (error: unknown) { + this.logger.error('Failed to authenticate widget:', error); + // Consider showing a user-friendly notification }
150-153: Maintain consistent error handlingFor consistency with the previous suggestion, consider applying the same error handling improvements here.
try { const widgetAuthData = await this.ajax.post<WidgetAuthResponse>( this.WIDGET_AUTH_ENDPOINT ); this.widgetAuthToken = widgetAuthData.token; -} catch (error) { - this.logger.error(error); +} catch (error: unknown) { + this.logger.error('Failed to get widget token:', error); + // Consider showing a user-friendly notification }
Line range hint
1-173: Well-structured service transition to ember-fetchThe migration from ember-ajax to ember-fetch is implemented consistently throughout the service, with proper typing and maintained functionality. The service maintains its single responsibility of handling Freshdesk integration.
app/services/ajax.ts (1)
287-296: Consider enhancing error response typing.The error response merging could benefit from more specific types to prevent potential type conflicts.
Consider this approach:
+interface ErrorResponseBody { + message?: string; + [key: string]: unknown; +} +type ErrorResponse = Response & ErrorResponseBody & { + payload: unknown; +}; - const errorResponse = { + const errorResponse: ErrorResponse = { ...response, ...(typeof parsedResponse === 'object' ? parsedResponse : { message: parsedResponse }), payload: parsedResponse, };app/services/configuration.ts (2)
72-75: Consider adding proper type for the session service.While the ajax service has been properly typed with
IreneAjaxService, the session service is still usingany. Consider adding proper type definitions for better type safety.
132-134: Consider enhancing error handling and value assignment.The ajax requests are properly typed, but there are two areas for improvement:
- Error handling could be more specific by catching
AjaxErrorinstead of generic errors- The use of
||=might ignore valid falsy values (likefalseor0)Example improvement:
try { const data = await this.ajax.request<FrontendConfigResponse>( this.frontendConfigEndpoint ); // Use direct assignment instead of ||= to avoid ignoring falsy values this.frontendData.hide_poweredby_logo = data.hide_poweredby_logo; } catch (error) { if (error instanceof AjaxError) { this.logger.error(`Failed to fetch frontend config: ${error.message}`, error); throw error; } throw error; }Also applies to: 204-206, 228-230
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (82)
app/adapters/file-report.ts(1 hunks)app/authenticators/base.ts(2 hunks)app/authenticators/irene.ts(1 hunks)app/authenticators/saml2.ts(1 hunks)app/components/account-settings/developer-settings/personal-token-list/delete-token/index.ts(3 hunks)app/components/account-settings/developer-settings/personal-token-list/index.ts(3 hunks)app/components/account-settings/general/select-language/index.ts(3 hunks)app/components/account-settings/security/multi-factor-auth/index.ts(16 hunks)app/components/account-settings/security/password-change/index.ts(4 hunks)app/components/analysis-risk/override-edit-drawer/override-form/index.ts(0 hunks)app/components/api-filter/index.ts(3 hunks)app/components/app-monitoring/version-table/actions/index.ts(2 hunks)app/components/appknox-wrapper/index.ts(0 hunks)app/components/attachment-detail/index.ts(1 hunks)app/components/dynamicscan-automation-upselling-feature/index.ts(1 hunks)app/components/file-details/api-scan/captured-apis/footer/index.ts(2 hunks)app/components/file-details/api-scan/captured-apis/index.ts(3 hunks)app/components/file-details/dynamic-scan/action/drawer/device-pref-table/index.ts(0 hunks)app/components/file-details/dynamic-scan/action/drawer/index.ts(2 hunks)app/components/file-details/dynamic-scan/action/index.ts(3 hunks)app/components/file-details/dynamic-scan/drawer-old/index.ts(2 hunks)app/components/file-details/dynamic-scan/results/index.ts(0 hunks)app/components/file-details/manual-scan/index.ts(3 hunks)app/components/file-details/manual-scan/request-access/index.ts(2 hunks)app/components/file-details/static-scan/index.ts(3 hunks)app/components/file-details/summary/file-tags/index.ts(2 hunks)app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts(2 hunks)app/components/file-details/vulnerability-analysis/header/index.ts(2 hunks)app/components/file/report-drawer/sbom-reports/sample/index.ts(1 hunks)app/components/file/report-drawer/va-reports/report-item/index.ts(2 hunks)app/components/home-page/index.ts(0 hunks)app/components/organization-analytics/app-scan-chart/index.ts(2 hunks)app/components/organization-analytics/index.ts(2 hunks)app/components/organization-billing/invoice-list/download-action/index.ts(2 hunks)app/components/organization-billing/subscription/index.ts(3 hunks)app/components/organization-invitation-list/invite-delete/index.ts(1 hunks)app/components/organization-invitation-list/invite-resend/index.ts(0 hunks)app/components/organization-team/create-team/index.ts(0 hunks)app/components/partner/client-report-download/index.js(1 hunks)app/components/project-preferences-old/provider/index.ts(2 hunks)app/components/project-preferences/provider/index.ts(2 hunks)app/components/project-settings/analysis-settings/toggle-analysis/index.ts(2 hunks)app/components/project-settings/analysis-settings/vulnerability-list/index.ts(3 hunks)app/components/project-settings/general-settings/dynamicscan-automation-settings/index.ts(2 hunks)app/components/project-settings/general-settings/github-project/index.ts(3 hunks)app/components/project-settings/view-scenario/index.ts(0 hunks)app/components/public-api-docs/index.ts(2 hunks)app/components/security/analysis-details/attachments/index.ts(4 hunks)app/components/security/analysis-details/index.ts(2 hunks)app/components/security/analysis-list/header/index.ts(2 hunks)app/components/security/analysis-list/table/action/index.ts(2 hunks)app/components/security/analysis-report-btn/index.ts(3 hunks)app/components/security/download-app/index.ts(1 hunks)app/components/security/file-details-actions/index.ts(4 hunks)app/components/security/file-search-list/download/index.ts(2 hunks)app/components/security/purge-api-analysis/index.ts(1 hunks)app/components/system-status/index.ts(3 hunks)app/components/user-login/index.ts(3 hunks)app/components/user-login/recover-password/index.ts(3 hunks)app/components/user-login/reset-password/index.ts(3 hunks)app/components/user-registration/via-login-page/index.ts(3 hunks)app/components/user-registration/via-org-invite/index.ts(3 hunks)app/components/user-registration/via-partner-invite/index.ts(4 hunks)app/models/vulnerability-preference.ts(1 hunks)app/routes/authenticated/dashboard/organization-settings/index.ts(1 hunks)app/routes/authenticated/github-cloud/redirect.ts(2 hunks)app/routes/authenticated/payment-success.ts(1 hunks)app/routes/invite.ts(1 hunks)app/services/ajax.js(0 hunks)app/services/ajax.ts(1 hunks)app/services/configuration.ts(6 hunks)app/services/devicefarm.ts(2 hunks)app/services/freshdesk.ts(3 hunks)app/services/network.ts(0 hunks)app/services/oidc.ts(5 hunks)app/services/organization.ts(1 hunks)app/services/websocket.ts(3 hunks)package.json(0 hunks)tests/integration/components/file-details/dynamic-scan/manual-test.js(5 hunks)tests/integration/components/file-details/summary-test.js(1 hunks)tests/integration/components/file-details/vulnerability-analysis-details/edit-analysis-button-test.js(2 hunks)tests/integration/components/partner/client-report-download-test.js(1 hunks)
⛔ Files not processed due to max files limit (8)
- tests/integration/components/project-settings/general-settings/dynamicscan-automation-settings/index-test.js
- tests/integration/components/security/analysis-details-test.js
- tests/integration/components/user-login/recover-password-test.js
- tests/integration/components/via-org-invite-test.js
- tests/integration/components/via-partner-invite-test.js
- tests/unit/services/ajax-test.js
- tsconfig.json
- types/irene/index.d.ts
💤 Files with no reviewable changes (11)
- app/components/file-details/dynamic-scan/results/index.ts
- app/components/appknox-wrapper/index.ts
- app/components/file-details/dynamic-scan/action/drawer/device-pref-table/index.ts
- app/components/organization-team/create-team/index.ts
- app/components/analysis-risk/override-edit-drawer/override-form/index.ts
- package.json
- app/components/home-page/index.ts
- app/components/project-settings/view-scenario/index.ts
- app/components/organization-invitation-list/invite-resend/index.ts
- app/services/ajax.js
- app/services/network.ts
🚧 Files skipped from review as they are similar to previous changes (63)
- app/authenticators/irene.ts
- app/components/security/download-app/index.ts
- app/routes/invite.ts
- app/components/file-details/api-scan/captured-apis/footer/index.ts
- app/components/organization-analytics/index.ts
- app/components/organization-invitation-list/invite-delete/index.ts
- app/services/devicefarm.ts
- tests/integration/components/partner/client-report-download-test.js
- app/components/user-login/recover-password/index.ts
- app/services/organization.ts
- app/components/file-details/vulnerability-analysis/header/index.ts
- tests/integration/components/file-details/summary-test.js
- app/components/security/file-search-list/download/index.ts
- app/components/project-settings/general-settings/dynamicscan-automation-settings/index.ts
- app/components/security/analysis-list/table/action/index.ts
- app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts
- app/components/file-details/manual-scan/request-access/index.ts
- app/components/organization-billing/invoice-list/download-action/index.ts
- app/components/file-details/static-scan/index.ts
- app/components/security/analysis-list/header/index.ts
- app/components/project-settings/analysis-settings/toggle-analysis/index.ts
- app/components/file-details/dynamic-scan/action/drawer/index.ts
- app/components/partner/client-report-download/index.js
- app/components/file-details/dynamic-scan/drawer-old/index.ts
- app/components/security/purge-api-analysis/index.ts
- app/components/dynamicscan-automation-upselling-feature/index.ts
- app/services/websocket.ts
- app/components/public-api-docs/index.ts
- app/components/organization-billing/subscription/index.ts
- app/components/file-details/summary/file-tags/index.ts
- app/components/account-settings/developer-settings/personal-token-list/index.ts
- app/components/attachment-detail/index.ts
- app/components/security/file-details-actions/index.ts
- tests/integration/components/file-details/vulnerability-analysis-details/edit-analysis-button-test.js
- app/components/api-filter/index.ts
- app/routes/authenticated/dashboard/organization-settings/index.ts
- app/components/account-settings/security/password-change/index.ts
- app/components/user-registration/via-partner-invite/index.ts
- app/services/oidc.ts
- app/components/security/analysis-details/index.ts
- app/components/account-settings/developer-settings/personal-token-list/delete-token/index.ts
- app/components/system-status/index.ts
- app/components/file-details/api-scan/captured-apis/index.ts
- app/routes/authenticated/payment-success.ts
- app/components/security/analysis-report-btn/index.ts
- app/components/file/report-drawer/sbom-reports/sample/index.ts
- app/authenticators/base.ts
- app/components/account-settings/general/select-language/index.ts
- app/components/user-login/reset-password/index.ts
- app/components/project-preferences/provider/index.ts
- app/components/project-preferences-old/provider/index.ts
- app/adapters/file-report.ts
- app/components/app-monitoring/version-table/actions/index.ts
- app/components/organization-analytics/app-scan-chart/index.ts
- app/components/file/report-drawer/va-reports/report-item/index.ts
- app/authenticators/saml2.ts
- app/components/file-details/dynamic-scan/action/index.ts
- app/components/file-details/manual-scan/index.ts
- app/components/project-settings/general-settings/github-project/index.ts
- app/components/security/analysis-details/attachments/index.ts
- app/components/user-registration/via-org-invite/index.ts
- app/components/user-login/index.ts
- app/components/user-registration/via-login-page/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/services/ajax.ts
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (20)
tests/integration/components/file-details/dynamic-scan/manual-test.js (3)
Line range hint
109-113: LGTM: Clean request body parsing implementation.The change from URL-encoded string manipulation to JSON parsing is a good improvement that aligns with modern API practices.
1013-1013: LGTM: Consistent enum usage.The use of ENUMS.DEVICE_TYPE.PHONE_REQUIRED is consistent and ensures type safety across assertions.
Also applies to: 1627-1627
Line range hint
1613-1627: LGTM: Comprehensive device preference assertions.The assertions thoroughly verify both the device type selection and platform version constraints.
app/components/account-settings/security/multi-factor-auth/index.ts (4)
18-26: LGTM! Type definitions improve code safety.The addition of
IreneAjaxServicetype andTokenDatainterface enhances type safety and makes the code more maintainable.
31-32: LGTM! Service type declaration is now properly typed.The
ajaxservice is now properly typed withIreneAjaxServiceinstead ofany, which aligns with the PR's objective of improving type safety.
527-527: Consider removing or masking tokens from debug logs.Logging tokens, even in debug mode, can be a security concern. Consider masking or removing sensitive data from logs.
- debug('SwitchTOEmail: App OTP Token Data ' + tokenData?.token); + debug('SwitchTOEmail: App OTP Token Data [MASKED]');
Line range hint
179-183: LGTM! Proper type annotations for API responses.The POST requests are now properly typed with
TokenData, which improves type safety and makes the code more maintainable.Also applies to: 312-316
app/services/freshdesk.ts (2)
11-17: Well-structured type definitions!The
WidgetAuthResponsetype definition provides clear typing for the authentication response, improving type safety.
23-23: LGTM - Service injection updated correctlyThe change from NetworkService to IreneAjaxService aligns with the PR objective of transitioning to ember-fetch.
app/routes/authenticated/github-cloud/redirect.ts (4)
7-8: LGTM! Import changes align with the service transition.The new type imports from the ajax service enhance type safety.
10-13: LGTM! Well-defined response type.The
GithubResponsetype enhances type safety by explicitly defining the expected GitHub API response structure.
16-16: LGTM! Enhanced type safety for ajax service.The service declaration now uses the specific
IreneAjaxServicetype instead ofany.
35-35: Enhance error handling robustness.The current error handling might break if the error payload structure is unexpected.
Consider this more robust approach:
- this.notify.error(`Error Occured: ${(err as AjaxError).payload.message}`); + const errorMessage = err instanceof AjaxError + ? err.payload?.message ?? 'Unknown error occurred' + : 'Failed to connect to GitHub'; + this.notify.error(`Error Occurred: ${errorMessage}`);Also, note the typo: "Occured" should be "Occurred".
app/services/ajax.ts (3)
7-21: LGTM! Well-designed request options interface.The
RequestOptionsinterface is well-documented and provides comprehensive type safety for API requests.
144-144: Type the session service properly.Using
anytype for the session service bypasses TypeScript's type checking.Consider this more robust approach:
- @service declare session: any; + @service declare session: { + data: { + authenticated: { + b64token?: string; + }; + }; + };
280-280: Replace delete operator with undefined assignment.Using the delete operator can impact performance.
Consider this more performant approach:
- delete (fetchOptions as RequestOptions).namespace; + (fetchOptions as RequestOptions).namespace = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 280-280: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
app/components/project-settings/analysis-settings/vulnerability-list/index.ts (3)
44-44: LGTM! Good type safety improvement.Replacing
anywithIreneAjaxServicetype improves type safety and aligns with the PR objective of transitioning to ember-fetch.
193-196: LGTM! Proper form data handling.The implementation correctly:
- Uses the type-safe
put<VulnerabilityPreferenceResponse>method- Handles form data encoding
- Sets appropriate content type headers
32-38: 🛠️ Refactor suggestionUpdate response interface to match model types.
Based on past review comments, the interface should use consistent types:
interface VulnerabilityPreferenceResponse { id: string; comment: string; - risk: number | null; + risk: number; updated_by: string | null; updated_date: string | null; }Likely invalid or redundant comment.
app/services/configuration.ts (1)
20-70: Well-structured type definitions!The new TypeScript types are comprehensive, properly separated, and align well with the API responses. This improves type safety and maintainability.
Irene
|
||||||||||||||||||||||||||||||||||
| Project |
Irene
|
| Branch Review |
PD-1583-Remove-Ember-Ajax
|
| Run status |
|
| Run duration | 05m 13s |
| Commit |
|
| Committer | SmitGala |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
1
|
|
Flaky
|
0
|
|
Pending
|
0
|
|
Skipped
|
0
|
|
Passing
|
31
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/tests/dynamic-scan.spec.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| Dynamic Scan > it tests dynamic scan for an ipa file: 58061 |
Test Replay
Screenshots
|
|
No description provided.