Ember upgrade v5 #1527
Ember upgrade v5 #1527
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
npm warn config production Use WalkthroughThis pull request encompasses a comprehensive set of changes across the Ember.js application, focusing on modernizing and streamlining the codebase. The modifications primarily involve transitioning from traditional Ember run loop utilities to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (82)
⛔ Files not processed due to max files limit (9)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (81)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (39)
tests/unit/models/available-device-test.js (1)
8-14: Consider improving test coverage and naming.The current test could be more meaningful:
- The test name "it exists" is too generic
- The assertion only verifies object creation
Consider enhancing the test:
- test('it exists', function (assert) { + test('should create available-device model with default attributes', function (assert) { const model = this.owner .lookup('service:store') .createRecord('available-device'); assert.ok(!!model); + assert.strictEqual(model.constructor.modelName, 'available-device', 'has correct model name'); + // Add assertions for expected default attributes });app/routes/authenticated/dashboard/app-monitoring/monitoring-details/index.ts (1)
17-19: Consider adding error handling for the transition.While the transition logic is correct, consider adding error handling to gracefully handle failed transitions.
beforeModel() { if (!this.organization.selected?.features.app_monitoring) { - this.router.transitionTo('authenticated.dashboard.projects'); + return this.router.transitionTo('authenticated.dashboard.projects').catch((error) => { + console.error('Failed to transition:', error); + // Handle the error appropriately + }); } }app/routes/authenticated/dashboard/app-monitoring/monitoring-details.ts (1)
Line range hint
42-46: Consider expanding error handling cases.While the 404 error handling is good, consider handling other common error cases.
@action error(error: AdapterError) { - if (error?.errors && error.errors[0]?.status === '404') { - this.router.transitionTo('/not-found'); + if (!error?.errors?.length) { + return; } + const status = error.errors[0]?.status; + + switch (status) { + case '404': + return this.router.transitionTo('/not-found'); + case '403': + return this.router.transitionTo('/forbidden'); + case '500': + // Handle server errors + console.error('Server error:', error); + return this.router.transitionTo('/error'); + default: + // Handle other cases + console.error('Unexpected error:', error); + } }tests/unit/models/tag-test.js (1)
9-9: Use const for immutable references.While the removal of the run loop wrapper is good, consider using
constinstead ofletsince themodelvariable is not reassigned.- let model = this.owner.lookup('service:store').createRecord('tag'); + const model = this.owner.lookup('service:store').createRecord('tag');tests/unit/models/profile-test.js (1)
9-9: Consider usingconstfor consistency.The model variable isn't reassigned, so using
constwould be more appropriate and consistent with other test files.- let model = this.owner.lookup('service:store').createRecord('profile'); + const model = this.owner.lookup('service:store').createRecord('profile');tests/unit/models/project-team-test.js (1)
9-9: Consider usingconstfor consistency.The model variable isn't reassigned, so using
constwould be more appropriate and consistent with other test files.- let model = this.owner.lookup('service:store').createRecord('project-team'); + const model = this.owner.lookup('service:store').createRecord('project-team');tests/unit/models/security/file-test.js (1)
9-11: Consider usingconstinstead oflet.For consistency with other test files in this PR (like capturedapi-test.js), consider using
constsince the model reference isn't reassigned.- let model = this.owner + const model = this.ownertests/unit/models/attachment-test.js (1)
9-9: Consider multi-line formatting for consistency.For consistency with other test files in this PR, consider using the same multi-line format for model creation.
- const model = this.owner.lookup('service:store').createRecord('attachment'); + const model = this.owner + .lookup('service:store') + .createRecord('attachment');tests/unit/models/proxy-settings-test.js (1)
5-5: Align model name in test description with actual model name.The test description uses 'proxy setting' while the model name is 'proxy-setting'. Consider updating the test description for consistency:
-module('Unit | Model | proxy setting', function (hooks) { +module('Unit | Model | proxy-setting', function (hooks) {Also applies to: 11-11
tests/unit/models/analytics/recent-issue-test.js (1)
9-11: Consider usingconstinstead oflet.Since the
modelvariable is not reassigned, usingconstwould be more appropriate and consistent with other test files.- let model = this.owner + const model = this.ownertests/unit/models/security/vulnerability-test.js (1)
9-11: Consider usingconstinstead oflet.Since the
modelvariable is not reassigned, usingconstwould be more appropriate and consistent with other test files.- let model = this.owner + const model = this.ownertests/unit/models/plan-test.js (2)
11-11: Consider removing deprecatedget()usage.In Ember Octane, direct property access is preferred over the
get()method.- assert.equal(plan.get('descriptionItems'), undefined, 'Description'); + assert.equal(plan.descriptionItems, undefined, 'Description');
Line range hint
1-1: Overall architectural feedback for the Ember v5 upgrade.The changes show a consistent effort to modernize the test suite by removing deprecated
runloop usage. To fully align with Ember Octane practices, consider:
- Using
constconsistently for model instances that aren't reassigned- Removing deprecated
get()method usage in favor of direct property access- Adding more comprehensive test cases beyond just "it exists" checks
These changes will help maintain consistency and leverage modern Ember features effectively.
tests/unit/models/pricing-test.js (1)
9-11: Consider updating to modern assertion syntax.While the removal of
runwrapper is correct, consider updating to modern assertion syntax:- assert.equal(pricing.get('descriptionItems'), undefined, 'Description'); + assert.strictEqual(pricing.descriptionItems, undefined, 'Description');This change would:
- Use
strictEqualfor explicit undefined checks- Use direct property access instead of
get()tests/unit/models/submission-test.js (1)
9-13: Consider updating to modern test patterns.The removal of
runwrapper and multi-line formatting improve readability. Consider these additional modernizations:- assert.equal(submission.get('hasReason'), false, 'Reason'); + assert.false(submission.hasReason, 'Submission should not have reason by default');This would:
- Use the more specific
assert.false- Use direct property access instead of
get()- Provide a more descriptive assertion message
tests/helpers/start-app.js (1)
11-16: LGTM! Consider updating merge usage.The removal of the
runwrapper and direct application return is correct for Ember v5.Replace deprecated
mergewith object spread:-import { merge } from '@ember/polyfills'; - let attributes = merge({}, config.APP); - attributes = merge(attributes, attrs); + let attributes = { + ...config.APP, + autoboot: true, + ...attrs + };This change:
- Removes dependency on deprecated
@ember/polyfills- Uses modern object spread syntax
- Simplifies the code by combining all property assignments
tests/unit/models/api-scan-options-test.js (1)
1-1: Consider removing global ESLint disable comments.Instead of disabling ESLint rules globally, consider:
- Enabling
prettier/prettierand fixing formatting- Adding expect count using
assert.expect()- Using
strictEqualinstead ofequaltests/unit/models/personaltoken-test.js (1)
13-15: Consider using more robust date handling.Using
new Date()with string parameters can be unreliable across different locales and timezones.Consider using:
- const d = new Date('25 March 2015'); + const d = new Date(2015, 2, 25); // Month is 0-basedtests/unit/models/device-preference-test.js (1)
13-28: Consider adding edge case tests.While the current tests cover basic functionality, consider adding tests for:
- Empty platform version
- Invalid version formats
- Null/undefined scenarios
app/components/ak-select/before-option/index.ts (1)
29-34: Consider simplifying the focusInput implementation.The current implementation could be more concise by combining the condition with the focus call.
- runTask(this, () => { - if (this.args.autofocus !== false) { - el.focus(); - } - }); + runTask(this, () => this.args.autofocus !== false && el.focus());app/components/ak-drawer/index.ts (1)
56-65: Consider removing unnecessary 0ms delays.Both
backdropInsertedanddrawerInserteduserunTaskwith a 0ms delay. SincerunTaskalready ensures proper execution timing, these delays could be removed.- runTask( - this, - () => { - this.backdropElement = element; - element.style.opacity = '1'; - element.style.transition = 'opacity 225ms ease-in-out 0ms'; - }, - 0 - ); + runTask(this, () => { + this.backdropElement = element; + element.style.opacity = '1'; + element.style.transition = 'opacity 225ms ease-in-out 0ms'; + });Also applies to: 70-79
app/components/analysis-risk/override-edit-drawer/index.ts (1)
6-6: Consider simplifying the state update.While the transition to
ember-lifelineis good, wrapping a simple state assignment inrunTaskmight be unnecessary here since there's no specific timing requirement.- runTask(this, () => { - this.appBarData = appBarData; - }); + this.appBarData = appBarData;Also applies to: 68-71
app/components/ak-select/index.ts (1)
73-73: LGTM! Consider adding an error boundary.The transition to
runTaskfrom ember-lifeline is appropriate for the Ember v5 upgrade. However, consider adding error handling around the reposition action to gracefully handle potential failures.- runTask(this, () => select.actions.reposition()); + runTask(this, () => { + try { + select.actions.reposition(); + } catch (error) { + console.error('Failed to reposition select dropdown:', error); + } + });tests/unit/models/file-test.js (1)
14-75: Consider grouping related test assertions.The test contains multiple related assertions that could be better organized using QUnit's test grouping features.
Consider restructuring the test using nested modules:
- test('it passes', function (assert) { + module('File Model', function () { + test('initial state', function (assert) { const file = this.owner.lookup('service:store').createRecord('file', { id: 1 }); assert.equal(file.get('isManualRequested'), true, 'Manual Requested'); // ... other initial state tests + }); + + test('status text updates', function (assert) { + const file = this.owner.lookup('service:store').createRecord('file', { id: 1 }); // ... status text related tests + }); + + test('status setters', function (assert) { + const file = this.owner.lookup('service:store').createRecord('file', { id: 1 }); // ... status setter tests + }); });app/components/ak-tooltip/index.ts (2)
36-42: Improve type safety in willDestroy.The type casting to
EmberRunTimercould be handled more safely.Consider this safer implementation:
- if (this.runShowToolipLater) { - cancelTask(this, this.runShowToolipLater as EmberRunTimer); - } - - if (this.runHideToolipLater) { - cancelTask(this, this.runHideToolipLater as EmberRunTimer); - } + const cancelTooltipTask = (timer?: EmberRunTimer | number) => { + if (timer && typeof timer === 'object') { + cancelTask(this, timer); + } + }; + + cancelTooltipTask(this.runShowToolipLater); + cancelTooltipTask(this.runHideToolipLater);
77-80: Improve type safety in handleHideTooltip.Similar to willDestroy, the type casting here could be handled more safely.
- const cancelled = cancelTask( - this, - this.runShowToolipLater as EmberRunTimer - ); + const cancelled = this.runShowToolipLater && typeof this.runShowToolipLater === 'object' + ? cancelTask(this, this.runShowToolipLater) + : false;app/components/appknox-wrapper/onboarding-guide/index.ts (1)
108-110: Consider cleanup on component destruction.While the change to
runTaskis appropriate, consider cleaning up any pending tasks when the component is destroyed.+ mountIFrameTask?: EmberRunTimer; + @action handleIframeInsertion() { - runTask(this, () => { + this.mountIFrameTask = runTask(this, () => { this.mountIFrame = true; }); } + + willDestroy() { + super.willDestroy(); + + if (this.mountIFrameTask) { + cancelTask(this, this.mountIFrameTask); + } + }app/components/project-settings/general-settings/add-project-team/table/index.ts (1)
Line range hint
121-134: Consider enhancing error handling in fetchOrganizationTeams task.The current error handling could be improved by:
- Logging the error for debugging
- Resetting the response state on error
- Adding retry logic for transient failures
fetchOrganizationTeams = task({ drop: true }, async () => { try { const queryParams = { limit: this.limit, offset: this.offset, q: this.searchQuery, exclude_project: this.args.project?.id, }; this.orgTeamRecordResponse = (await this.store.query( 'organization-team', queryParams )) as OrganizationTeamRecordModelArray; } catch (e) { + console.error('Failed to fetch organization teams:', e); + this.orgTeamRecordResponse = null; this.notify.error(parseError(e, this.intl.t('pleaseTryAgain'))); } });app/components/organization-member/list/add-to-team/index.ts (1)
6-6: LGTM! Correct migration to ember-lifeline.The migration from
debouncetodebounceTaskfollows the correct pattern. The debounced method exists and the parameters are properly ordered.Consider documenting cleanup behavior.
ember-lifeline automatically cleans up pending tasks when the component is destroyed, which is an improvement over manual cleanup with
@ember/runloop.Add a comment above the class to document this behavior:
+// Note: ember-lifeline automatically cancels pending debounced tasks on component destruction export default class AddToTeamComponent extends Component<AddToTeamComponentSignature> {Also applies to: 68-73
app/components/project-settings/general-settings/add-project-collaborator/table/index.ts (1)
11-11: LGTM! Correct migration to ember-lifeline.The migration from
debouncetodebounceTaskfollows the correct pattern.Consider renaming the debounced method for clarity.
The method name
updateSearchQuerycould be more specific about its side effects.- updateSearchQuery() { + updateSearchQueryAndRefetch() { this.searchQuery = this.query; this.fetchOrganizationMembers.perform(); }Also applies to: 87-87
app/components/security/project-search-list/index.ts (1)
12-12: LGTM! Consider extracting the debounce delay to a constant.The transition from
@ember/runlooptoember-lifelineis correctly implemented. ThedebounceTaskusage with string method name follows the recommended pattern.Consider extracting the magic number
500to a class constant for better maintainability:export default class SecurityProjectSearchListComponent extends Component<SecurityProjectSearchListSignature> { + private readonly SEARCH_DEBOUNCE_MS = 500; // ... @action searchProjectsQuery(event: Event): void { const query = (event.target as HTMLInputElement).value; - debounceTask(this, 'searchProjects', query, 500); + debounceTask(this, 'searchProjects', query, this.SEARCH_DEBOUNCE_MS); }Also applies to: 96-96
app/components/organization-team/project-list/index.ts (1)
5-5: Consider standardizing debounce delays across components.The transition to
ember-lifelineis correctly implemented. However, I notice that multiple components use the same 500ms debounce delay.Consider creating a shared constant or configuration value for the debounce delay to ensure consistency across all search components:
// app/constants/timing.ts export const SEARCH_DEBOUNCE_MS = 500;Then import and use this constant in all components:
+import { SEARCH_DEBOUNCE_MS } from 'app/constants/timing'; // ... -debounceTask(this, 'setSearchQuery', (event.target as HTMLInputElement).value, 500); +debounceTask(this, 'setSearchQuery', (event.target as HTMLInputElement).value, SEARCH_DEBOUNCE_MS);Also applies to: 117-121
app/components/organization/service-account/section/access-token/index.ts (1)
8-8: LGTM! Consider adding return type to the callback.The transition from
next()torunTask()is correctly implemented. The scheduling ofsetCreateServiceAccountExpiryafter the constructor maintains the same behavior while using the new ember-lifeline API.Consider adding a return type to the callback for better type safety:
- runTask(this, () => this.setCreateServiceAccountExpiry()); + runTask(this, (): void => this.setCreateServiceAccountExpiry());Also applies to: 42-42
app/components/sbom/scan-details/component-list/index.ts (1)
7-7: LGTM! Consider standardizing debounce time.The transition to
ember-lifeline'sdebounceTaskis a good improvement as it provides better task management and automatic cleanup. The implementation is correct.Consider extracting the debounce time (500ms) to a constant or configuration value to maintain consistency across components. This would make it easier to adjust the debounce timing globally if needed.
+const SEARCH_DEBOUNCE_MS = 500; + @action searchSbomComponentForQuery(event: Event) { const query = (event.target as HTMLInputElement).value; - debounceTask(this, 'setSearchQuery', query, 500); + debounceTask(this, 'setSearchQuery', query, SEARCH_DEBOUNCE_MS); }Also applies to: 138-138
app/components/system-status/index.ts (1)
Line range hint
147-157: Consider adding retry logic for websocket health check.The current implementation only attempts the health check once. Consider adding retry logic with exponential backoff for better reliability.
triggerWebsocketHealthCheck = task(async (socketId: string) => { + const MAX_RETRIES = 3; + const INITIAL_TIMEOUT = 1000; + + for (let retry = 0; retry < MAX_RETRIES; retry++) { this.socket?.emit('subscribe', { room: socketId }); - // wait for user to join room - await timeout(1000); + // wait with exponential backoff + await timeout(INITIAL_TIMEOUT * Math.pow(2, retry)); await this.ajax.post('/api/websocket_health_check', { data: {} }); - // wait for socket to notify - await timeout(1000); + try { + await timeout(INITIAL_TIMEOUT); + return; + } catch (error) { + if (retry === MAX_RETRIES - 1) throw error; + } + } });app/components/organization/service-account/add-project/list/index.ts (1)
Line range hint
116-146: Consider enhancing error handling in addSelectedProjects task.The current implementation counts errors but doesn't provide specific feedback about which projects failed to add.
addSelectedProjects = task(async () => { const selectedProjects = Object.values(this.selectedProjects); - let addProjectErrorCount = 0; + const errors: Array<{ project: ProjectModel; error: Error }> = []; if (selectedProjects.length === 0) { return; } for (let i = 0; i < selectedProjects.length; i++) { const project = selectedProjects[i] as ProjectModel; - const hasErrored = await waitForPromise( - this.serviceAccount.addProjectToServiceAccount.perform( - project, - this.args.serviceAccount - ) - ); + try { + await waitForPromise( + this.serviceAccount.addProjectToServiceAccount.perform( + project, + this.args.serviceAccount + ) + ); + } catch (error) { + errors.push({ project, error }); + } } - if (addProjectErrorCount === 0) { + if (errors.length === 0) { // close the drawer this.args.drawerCloseHandler(); this.notify.success( this.intl.t('serviceAccountModule.addProjectSuccessMsg') ); // refresh selected project list this.args.refreshSelectedProjects(); + } else { + // Provide detailed error feedback + errors.forEach(({ project, error }) => { + this.notify.error( + this.intl.t('serviceAccountModule.addProjectErrorMsg', { + projectName: project.name, + error: parseError(error, this.intl.t('pleaseTryAgain')) + }) + ); + }); } });app/components/organization-member/list/index.ts (1)
4-4: Consider reorganizing related methods.The
setSearchQuerymethod andsearchUserForQueryaction are related but separated in the file. Consider moving them closer together for better code organization.@action searchUserForQuery(event: Event): void { debounceTask( this, 'setSearchQuery', (event.target as HTMLInputElement).value, 500 ); } + /* Set debounced searchQuery */ + setSearchQuery(query: string) { + const { user_limit, show_inactive_user } = this.args.queryParams; + + this.fetchUsers.perform(user_limit, 0, query, show_inactive_user); + } setRouteQueryParams( limit?: number, offset?: number, query?: string, showInActiveUser?: boolean ) { - /* Set debounced searchQuery */ - setSearchQuery(query: string) { - const { user_limit, show_inactive_user } = this.args.queryParams; - - this.fetchUsers.perform(user_limit, 0, query, show_inactive_user); - }Also applies to: 159-164, 168-173
app/components/sbom/app-list/index.ts (1)
6-6: Consider documenting the ember-lifeline migration pattern.The systematic migration from
@ember/runlooptoember-lifelineacross components improves task management and cleanup. Consider:
- Adding a migration guide to the documentation
- Creating an ESLint rule to prevent future usage of
@ember/runloop- Adding tests to verify proper cleanup of debounced tasks
This will help maintain consistency as more components are migrated.
Also applies to: 5-5, 4-4, 7-7
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (1)
254-254: Modernization of array access methods looks good.The changes consistently replace deprecated Ember array methods (
lastObject,firstObject) with modern JavaScript array methods (at(-1), array indexing). This improves code maintainability and aligns with current JavaScript best practices.Consider adding a type guard when using
at()since it might returnundefined:-const analysis = this.file.analyses.at(-1); +const analysis = this.file.analyses.at(-1); +if (!analysis) { + throw new Error('Expected at least one analysis'); +}Also applies to: 338-338, 420-420, 502-502, 570-570, 626-626, 801-801, 845-845, 901-901, 964-964, 1081-1081, 1212-1212, 1403-1407, 1812-1816
📜 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)
.stylelintrc.js(1 hunks)app/adapters/jira-repo.ts(1 hunks)app/components/ak-drawer/index.ts(3 hunks)app/components/ak-select/before-option/index.ts(2 hunks)app/components/ak-select/index.ts(2 hunks)app/components/ak-tooltip/index.ts(4 hunks)app/components/analysis-risk/override-edit-drawer/index.ts(2 hunks)app/components/app-monitoring/settings/index.ts(2 hunks)app/components/appknox-wrapper/onboarding-guide/index.ts(2 hunks)app/components/file-compare/table/index.ts(2 hunks)app/components/file-details/dynamic-scan/action/expiry/index.ts(2 hunks)app/components/github-account/index.js(1 hunks)app/components/organization-member/list/add-to-team/index.ts(2 hunks)app/components/organization-member/list/index.ts(2 hunks)app/components/organization-member/list/member-details/index.ts(2 hunks)app/components/organization-team/add-team-member/index.ts(2 hunks)app/components/organization-team/add-team-project/index.ts(2 hunks)app/components/organization-team/index.ts(2 hunks)app/components/organization-team/invitation-list/index.ts(2 hunks)app/components/organization-team/member-list/index.ts(2 hunks)app/components/organization-team/project-list/index.ts(2 hunks)app/components/organization/service-account/add-project/list/index.ts(2 hunks)app/components/organization/service-account/create/index.ts(2 hunks)app/components/organization/service-account/section/access-token/index.ts(2 hunks)app/components/organization/service-account/section/select-project/list/index.ts(2 hunks)app/components/partner/client-report-download/index.js(1 hunks)app/components/partner/client-uploads-stat-chart/index.js(1 hunks)app/components/project-settings/general-settings/add-project-collaborator/table/index.ts(2 hunks)app/components/project-settings/general-settings/add-project-team/table/index.ts(2 hunks)app/components/project-settings/general-settings/jira-project/index.ts(0 hunks)app/components/sbom/app-list/index.ts(2 hunks)app/components/sbom/scan-details/component-list/index.ts(2 hunks)app/components/security/project-search-list/index.ts(2 hunks)app/components/storeknox/discover/requested-apps/index.ts(1 hunks)app/components/system-status/index.ts(2 hunks)app/routes/authenticated/dashboard/app-monitoring/monitoring-details.ts(1 hunks)app/routes/authenticated/dashboard/app-monitoring/monitoring-details/history.ts(2 hunks)app/routes/authenticated/dashboard/app-monitoring/monitoring-details/index.ts(2 hunks)app/services/body-class.js(2 hunks)app/services/websocket.ts(2 hunks)config/environment.js(1 hunks)lib/ember-django-adapter/package.json(1 hunks)package.json(6 hunks)tests/acceptance/file-compare-test.js(1 hunks)tests/helpers/destroy-app.js(1 hunks)tests/helpers/start-app.js(1 hunks)tests/integration/components/file-details/vulnerability-analysis-details/index-test.js(14 hunks)tests/integration/components/plus-n-list-test.js(1 hunks)tests/unit/initializers/irene-test.js(1 hunks)tests/unit/models/analysis-test.js(1 hunks)tests/unit/models/analytics/recent-issue-test.js(1 hunks)tests/unit/models/api-scan-options-test.js(1 hunks)tests/unit/models/attachment-test.js(1 hunks)tests/unit/models/available-device-test.js(1 hunks)tests/unit/models/capturedapi-test.js(1 hunks)tests/unit/models/coupon-test.js(1 hunks)tests/unit/models/device-preference-test.js(1 hunks)tests/unit/models/device-test.js(1 hunks)tests/unit/models/file-test.js(1 hunks)tests/unit/models/invoice-test.js(1 hunks)tests/unit/models/manualscan-test.js(1 hunks)tests/unit/models/mfa-test.js(1 hunks)tests/unit/models/organization-mfa-test.js(1 hunks)tests/unit/models/organization-namespace-test.js(1 hunks)tests/unit/models/organization-user-test.js(1 hunks)tests/unit/models/owasp-test.js(1 hunks)tests/unit/models/owaspmobile2024-test.js(1 hunks)tests/unit/models/personaltoken-test.js(1 hunks)tests/unit/models/plan-test.js(1 hunks)tests/unit/models/pricing-test.js(1 hunks)tests/unit/models/profile-test.js(1 hunks)tests/unit/models/project-team-test.js(1 hunks)tests/unit/models/project-test.js(1 hunks)tests/unit/models/proxy-settings-test.js(1 hunks)tests/unit/models/security/analysis-test.js(1 hunks)tests/unit/models/security/attachment-test.js(1 hunks)tests/unit/models/security/file-test.js(1 hunks)tests/unit/models/security/project-test.js(1 hunks)tests/unit/models/security/vulnerability-test.js(1 hunks)tests/unit/models/submission-test.js(1 hunks)tests/unit/models/subscription-test.js(1 hunks)tests/unit/models/tag-test.js(1 hunks)
⛔ Files not processed due to max files limit (9)
- tests/unit/models/team-invitation-test.js
- tests/unit/models/team-member-test.js
- tests/unit/models/team-project-test.js
- tests/unit/models/unknown-analysis-status-test.js
- tests/unit/models/user-test.js
- tests/unit/models/vulnerability-preference-test.js
- tests/unit/models/vulnerability-test.js
- tests/unit/routes/authenticated/security/file-test.js
- tests/unit/serializers/mfa-test.js
💤 Files with no reviewable changes (1)
- app/components/project-settings/general-settings/jira-project/index.ts
✅ Files skipped from review due to trivial changes (4)
- tests/unit/models/organization-namespace-test.js
- tests/unit/models/organization-user-test.js
- tests/unit/models/owaspmobile2024-test.js
- tests/unit/models/security/attachment-test.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (61)
tests/unit/models/security/analysis-test.js (2)
5-5: LGTM! Clean formatting.The spacing improvement in the module declaration enhances readability.
8-12: Great modernization of the test implementation!The changes effectively:
- Remove deprecated run loop usage
- Use modern test patterns with direct owner lookup
- Improve readability with method chaining
This aligns well with Ember v5 best practices.
tests/unit/models/available-device-test.js (2)
5-5: LGTM! Modern module declaration.The module declaration follows the recommended pattern for Ember.js tests.
8-11: LGTM! Modern Ember.js patterns applied.The changes align well with Ember 5 best practices:
- Removed unnecessary run loop wrapper
- Direct store lookup and record creation
- Proper use of
constapp/routes/authenticated/dashboard/app-monitoring/monitoring-details/index.ts (1)
3-3: LGTM! Good practice using RouterService.The addition of RouterService and proper type imports aligns with modern Ember.js practices and improves type safety.
Also applies to: 12-12
app/routes/authenticated/dashboard/app-monitoring/monitoring-details/history.ts (2)
3-3: LGTM! Consistent implementation with index route.The RouterService implementation maintains consistency across routes, which is excellent for maintainability.
Also applies to: 12-12
17-19: Consider adding error handling for the transition.Similar to the index route, consider adding error handling for the transition.
app/routes/authenticated/dashboard/app-monitoring/monitoring-details.ts (1)
21-23: LGTM! Consistent implementation across routes.The RouterService usage maintains consistency with child routes.
tests/unit/models/security/project-test.js (2)
5-5: LGTM! Modern module structure.The module declaration follows current Ember.js testing patterns with proper hooks setup.
8-11: Clean implementation using modern Ember.js patterns.Good job removing the
runwrapper and using direct store lookup. This aligns well with Ember.js v5 best practices.Let's verify there are no remaining run loop dependencies in related files:
✅ Verification successful
Verified: Run loop removal is safe and follows best practices
The codebase scan confirms no run loop dependencies in security-related modules. The direct store lookup pattern used is the correct approach for modern Ember.js applications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining @ember/runloop imports or run usages in related test files echo "Checking for remaining runloop usage in security-related test files..." rg -l "@ember/runloop|run\(" "tests/unit/models/security/" echo "Checking for potential run loop timing issues in the model itself..." rg -l "runLoop|run\(" "app/models/security/"Length of output: 401
Script:
#!/bin/bash # First, verify if directories exist echo "Checking directory structure..." fd -t d "security" --max-depth 3 echo "\nSearching for run loop patterns (case-insensitive)..." rg -il "(@ember/runloop|ember[- ]run|run[. (]|runloop|run\.)" tests/ app/ echo "\nChecking specific model imports..." rg -l "import.*from.*@ember" "tests/unit/models/security/project-test.js" || echo "File not found"Length of output: 1157
tests/unit/models/mfa-test.js (2)
8-10: LGTM! Good modernization of the test case.The changes align well with modern Ember practices:
- Removed the
runwrapper in favor of direct model instantiation- Using
this.owner.lookupis the recommended approach in Ember v5- Improved readability with consistent spacing
5-5: Verify formatting changes against style guide.The file has
eslint-disable prettier/prettierat the top, but includes formatting changes. Consider removing the eslint-disable if the new formatting is intended to comply with the style guide.✅ Verification successful
Formatting change is consistent with codebase style
The space between
functionand(hooks)follows the predominant style used across the model test files. The eslint-disable comment is common across test files and doesn't invalidate the formatting change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if other test files follow the same formatting pattern rg -l "module\('Unit \| Model \| .+', function \(hooks\) {" tests/unit/models/Length of output: 252
Script:
#!/bin/bash # Check module declarations in test files echo "=== Module declarations ===" rg "module\(" tests/unit/models/ echo -e "\n=== ESLint disable comments ===" rg "eslint-disable.*prettier" tests/unit/models/Length of output: 14666
tests/helpers/destroy-app.js (1)
2-2: LGTM! Removal of run wrapper aligns with Ember v5.The direct call to
application.destroy()without the run loop wrapper is the recommended approach in Ember v5.tests/unit/models/owasp-test.js (1)
9-9: LGTM! Modern Ember test patterns applied.The changes follow current Ember testing best practices:
- Direct store lookup via owner
- Removed unnecessary run loop wrapper
tests/unit/models/tag-test.js (1)
5-12: LGTM! Modern Ember test patterns applied.The changes follow current Ember testing best practices:
- Direct store lookup via owner
- Removed unnecessary run loop wrapper
tests/unit/models/coupon-test.js (1)
8-10: Clean modernization of test code!Good improvements:
- Removed deprecated
runloop usage- Used
constfor immutable model declaration- Simplified test setup with direct store lookup
tests/unit/models/device-test.js (1)
8-10: LGTM! Consistent with modernization pattern.Successfully removed deprecated
runloop usage while maintaining test functionality.tests/unit/models/project-team-test.js (1)
Line range hint
1-13: Well-executed modernization aligned with Ember v5!The changes successfully:
- Remove deprecated run loop usage
- Simplify test setup
- Align with the broader transition to ember-lifeline mentioned in other files
tests/unit/models/capturedapi-test.js (1)
5-5: LGTM! Changes align with modern Ember practices.The removal of
@ember/runloopand direct usage ofstore.createRecordfollows current Ember best practices. The code is now cleaner and more maintainable.Also applies to: 8-12
tests/unit/models/organization-mfa-test.js (1)
5-5: Well-structured test modernization!Good improvements:
- Removed deprecated
runwrapper- Using direct model creation via
this.owner.lookup()- Using
constfor immutable model reference- Consistent function declaration formatting
Also applies to: 8-11
tests/unit/initializers/irene-test.js (1)
9-13: LGTM! Removal ofrunwrapper aligns with Ember v5.The direct application creation without
runwrapper is the recommended approach in Ember v5. The sequence of creating the application and deferring readiness is maintained correctly.Let's verify there are no remaining
runusages in initializer tests:✅ Verification successful
Verified: No remaining runloop usage in initializer tests
The removal of the
runwrapper is complete and consistent across all initializer tests. No other instances of runloop usage were found in the tests/unit/initializers directory.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining @ember/runloop usage in initializer tests rg -l '@ember/runloop' 'tests/unit/initializers/'Length of output: 47
Script:
#!/bin/bash # First verify the directory exists and show test files echo "=== Checking test files ===" fd . 'tests/unit/initializers/' echo -e "\n=== Checking for any 'run' usage ===" # Search for run patterns with context, case-insensitive rg -i "\.run\(" 'tests/unit/initializers/' -A 2 || true rg -i "run\(" 'tests/unit/initializers/' -A 2 || true echo -e "\n=== Checking alternative import patterns ===" # Check for various import patterns rg "import .* from ['\"](ember|@ember).*run" 'tests/unit/initializers/' || trueLength of output: 523
tests/unit/models/api-scan-options-test.js (1)
8-18: LGTM! Clean test structure aligned with Ember v5.The removal of the run loop wrapper and direct usage of store service follows current Ember best practices. The test structure is clean and focused.
tests/unit/models/personaltoken-test.js (1)
8-21: LGTM! Clean test structure with proper store usage.The removal of run loop and usage of chained method calls improves readability while following Ember v5 patterns.
app/adapters/jira-repo.ts (2)
Line range hint
12-21: LGTM! Well-typed adapter implementation.The TypeScript types are properly used with ModelRegistry and Snapshot types, ensuring type safety.
17-17: Verify the impact of URL generation change.The change from using
snapshot.idtosnapshot.record.project.get('id')could affect record creation. Please ensure:
- All JIRA repo records are properly associated with projects
- No existing code relies on the previous URL format
✅ Verification successful
URL generation change is correct and safe ✅
The change to use
snapshot.record.project.get('id')is proper as it:
- Aligns with the model's required project relationship
- Follows ember-data conventions for belongsTo relationships
- Has no conflicting usage in tests or mock endpoints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any direct usage of jira-repo adapter or related URLs rg -l "jira-repo|/jira" --type ts --type js # Search for any test cases involving JIRA repo creation ast-grep --pattern 'createRecord("jira-repo"'Length of output: 706
Script:
#!/bin/bash # Check model definition and relationships cat app/models/jira-repo.ts # Examine test cases cat tests/unit/models/jira-repo-test.js # Check mirage config for URL patterns cat mirage/config.js | grep -A 5 -B 5 "jira"Length of output: 3508
tests/unit/models/device-preference-test.js (1)
8-11: LGTM! Proper test setup with intl service.The beforeEach hook correctly sets up the intl service, following Ember testing best practices.
tests/unit/models/invoice-test.js (1)
13-33: LGTM! Test modernization looks good.The changes appropriately remove the Ember run loop wrapper while maintaining the same test functionality. This aligns with modern Ember testing patterns and the PR's objective of upgrading to Ember v5.
tests/unit/models/manualscan-test.js (1)
10-46: LGTM! Well-structured test modernization.The changes demonstrate good practices:
- Consistent use of
strictEqualfor precise equality checks- Clean removal of run loop wrapper
- Well-organized test structure with clear spacing between assertions
app/components/ak-select/before-option/index.ts (2)
4-4: LGTM! Good migration to ember-lifeline.The import change appropriately brings in the new ember-lifeline utilities.
24-26: LGTM! Clean transition to scheduleTask.The change from
scheduleOncetoscheduleTaskis appropriate for the Ember v5 upgrade.tests/unit/models/project-test.js (1)
14-57: LGTM! Well-structured test modernization.The changes successfully:
- Remove run loop wrapper while maintaining test functionality
- Use consistent assertion formatting
- Maintain comprehensive test coverage for platform icons
tests/unit/models/subscription-test.js (1)
14-61: LGTM! Test refactoring improves readability.The removal of the
runwrapper from@ember/runloopsimplifies the test structure while maintaining the same test coverage and assertions. This change aligns well with modern Ember practices.app/components/github-account/index.js (1)
16-25: Verify property initialization timing.Moving
integratedUserinitialization from property declaration to constructor could affect access to this property in other lifecycle hooks or during component initialization.Run this script to check for any early access to
integratedUser:✅ Verification successful
Property initialization timing is correct and safe.
The tracked property is properly initialized in the constructor and there's no early access before initialization. The change follows Ember Octane best practices for component property initialization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential early access to integratedUser before constructor initialization ast-grep --pattern 'class $_ extends $_ { $$$ @tracked integratedUser; $$$ constructor() { $$$ } $$$ }' # Search for any direct references to integratedUser rg "integratedUser" -A 2 -B 2Length of output: 9760
app/components/ak-drawer/index.ts (1)
3-3: LGTM! Improved task management with ember-lifeline.The transition from
@ember/runlooptoember-lifelineenhances task management. TherunTaskutility provides automatic cleanup when the component is destroyed.Also applies to: 41-41
tests/unit/models/analysis-test.js (1)
6-127: LGTM! Well-structured test cases with good coverage.The test file demonstrates good practices with:
- Proper test setup and hooks
- Comprehensive coverage of model methods
- Clear test descriptions and assertions
- Good isolation of test cases
app/components/app-monitoring/settings/index.ts (1)
7-7: LGTM! Good migration to ember-lifeline.The change from
debouncetodebounceTaskaligns with modern Ember practices and improves component lifecycle management.Let's verify if the 500ms debounce timing aligns with other search implementations:
Also applies to: 99-99
✅ Verification successful
✓ Debounce timing of 500ms is consistent across search implementations
All search-related debounce implementations in the codebase use a consistent 500ms timing, confirming this is the established pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other debounce implementations to verify consistency rg -A 1 "debounceTask.*setSearchQuery|debounceTask.*search" --type tsLength of output: 704
app/components/project-settings/general-settings/add-project-team/table/index.ts (1)
10-11: LGTM! Good migration to ember-lifeline.The change from
debouncetodebounceTaskaligns with modern Ember practices.Also applies to: 88-88
app/components/organization/service-account/create/index.ts (1)
6-6: Verify potential race condition in setChangeset.While the migration to
runTaskis good, the current implementation might have a race condition ifsetChangesetis called multiple times in quick succession. Consider using a cancellable task or debouncing the changeset updates.@action setChangeset(changeset: BufferedChangeset) { const mergedChangeset = this.changeset?.merge(changeset) || changeset; - runTask(this, () => { - this.changeset = mergedChangeset; - }); + // Use debounceTask to prevent rapid updates + debounceTask(this, () => { + if (!this.isDestroying) { + this.changeset = mergedChangeset; + } + }, 100); return mergedChangeset; }Also applies to: 104-107
app/components/file-compare/table/index.ts (1)
5-5: LGTM! Correct migration to ember-lifeline.The migration from
latertorunTaskfollows the correct pattern and maintains the same timing behavior. The automatic cleanup of scheduled tasks by ember-lifeline is an improvement over manual cleanup with@ember/runloop.Also applies to: 153-157
app/components/storeknox/discover/requested-apps/index.ts (1)
53-53: LGTM! Good optimization to prevent unnecessary route transitions.The addition of
falseas the third parameter prevents updating route query parameters during the initial load, while still maintaining them during explicit user actions like pagination.app/components/file-details/dynamic-scan/action/expiry/index.ts (1)
7-7: LGTM! Good migration to ember-lifeline for better task management.The change from
latertorunTaskis a good improvement as ember-lifeline:
- Automatically cleans up scheduled tasks when the component is destroyed
- Prevents potential memory leaks
- Aligns with modern Ember practices for managing async operations
Also applies to: 124-125
app/components/organization-team/index.ts (1)
3-3: LGTM! Consistent migration to ember-lifeline for debounced operations.The change from
debouncetodebounceTaskmaintains the same functionality while providing better task cleanup. The consistent use of string method name ('setSearchQuery') follows ember-lifeline's best practices.Also applies to: 79-84
app/components/organization-team/member-list/index.ts (1)
5-5: LGTM! Maintains consistency with other components.The migration to
debounceTaskfollows the same pattern used in other components, ensuring a consistent approach to debounced operations across the codebase.Also applies to: 121-125
app/components/organization-member/list/member-details/index.ts (1)
5-5: LGTM! Consistent implementation across components.The transition to
ember-lifelinemaintains consistency with other components in the codebase.Also applies to: 130-134
app/components/system-status/index.ts (1)
5-5: LGTM! The transition to ember-lifeline's runTask is appropriate.The change improves task management and cleanup handling.
Also applies to: 50-50
app/components/organization/service-account/section/select-project/list/index.ts (1)
6-6: LGTM! Clean transition to ember-lifeline.The implementation correctly uses
debounceTaskand maintains consistent debounce timing with other components.Also applies to: 120-124
app/components/organization/service-account/add-project/list/index.ts (1)
5-5: LGTM! Clean transition to ember-lifeline.The implementation correctly uses
debounceTaskand maintains consistent debounce timing.Also applies to: 169-173
app/components/organization-team/add-team-member/index.ts (1)
6-6: LGTM! Correctly migrated to ember-lifeline.The migration from
@ember/runlooptoember-lifelinefollows the recommended pattern, maintaining the same debouncing behavior while providing better task management.Also applies to: 193-198
app/components/organization-team/add-team-project/index.ts (1)
5-5: LGTM! Consistent implementation across components.The migration pattern matches other components, maintaining codebase consistency.
Also applies to: 189-194
app/components/sbom/app-list/index.ts (1)
7-7: LGTM! Clean and concise implementation.The migration uses a more concise form of
debounceTaskwhile maintaining the same functionality.Also applies to: 168-168
app/services/websocket.ts (1)
5-5: LGTM! Good transition to ember-lifeline.The change from
@ember/runloop'sdebouncetoember-lifeline'sdebounceTaskis a good modernization step. Theember-lifelineaddon provides better task management and cleanup, helping prevent memory leaks and improving the overall reliability of async operations.Also applies to: 238-238
app/components/partner/client-uploads-stat-chart/index.js (1)
75-75: LGTM! Better initialization order.Moving the
currentTimelineinitialization to the constructor after extending dayjs with plugins ensures proper initialization order. This is a good practice as it guarantees that all required dayjs plugins are loaded before any timeline-related operations.Also applies to: 83-90
tests/acceptance/file-compare-test.js (1)
33-33: LGTM! Simplified test setup.The removal of the
vulnerabilitiesdestructuring simplifies the test setup by focusing only on the required endpoints. This change better aligns with the test's primary purpose of comparing files.config/environment.js (1)
152-152: LGTM! Enhanced security by disabling global application export.Setting
exportApplicationGlobaltofalseis a good security practice as it prevents exposing the application as a global variable.Let's verify that no external scripts depend on accessing the application globally:
✅ Verification successful
Confirmed: No external dependencies on global application access
Setting
exportApplicationGlobal: falseis safe as no code in the codebase attempts to access the application instance globally. All window object usage is for legitimate purposes like browser APIs and third-party integrations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for potential references to the global application object # Search for potential global application references in JavaScript/TypeScript files rg -t js -t ts "window\.(irene|app|application|App|Application)" -A 3Length of output: 71
Script:
#!/bin/bash # Broader search for global application references # Search for Ember's global export patterns and window access rg -t js -t ts -i "((window\[.[^]]+\])|window\.[\w]+|globalThis\.[\w]+|self\.[\w]+)" -A 2 # Search for environment configuration related to global export rg -t js "exportApplication(Global|)" -A 2 # Search for direct global assignments ast-grep --pattern 'window.$_ = $_'Length of output: 35247
lib/ember-django-adapter/package.json (1)
7-9: Dependencies updated appropriately for Ember 5.The added dependencies and their versions are correct for the Ember 5 upgrade:
- ember-data ~5.3.8 aligns with Ember 5
- ember-auto-import ^2.8.1 is the latest compatible version
- ember-cli-babel ^8.2.0 is maintained
app/services/body-class.js (1)
3-3: Good migration to ember-lifeline for better resource management.The change from
scheduleOncetoscheduleTaskis correct and brings several benefits:
- Automatic cleanup when the service is destroyed
- Consistent scheduling behavior
- Better memory management
Also applies to: 64-64
app/components/organization-team/invitation-list/index.ts (1)
5-5: Correct migration to ember-lifeline's debounceTask.The change from
debouncetodebounceTaskis well implemented:
- Maintains the same 500ms debounce delay
- Properly passes the method name as a string
- TypeScript types are preserved
Let's verify if this pattern is consistently applied across other components:
Also applies to: 73-77
✅ Verification successful
Migration to ember-lifeline's debounceTask is complete and consistent
The codebase shows a complete migration with no remaining @ember/runloop debounce usage. All components consistently use ember-lifeline's debounceTask with proper imports and implementation patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining uses of @ember/runloop debounce rg "import.*debounce.*from '@ember/runloop'" app/Length of output: 57
Script:
#!/bin/bash # Broader search for any remaining @ember/runloop imports echo "=== Checking for @ember/runloop imports ===" rg "from '@ember/runloop'" app/ # Search for debounce usage without import context echo -e "\n=== Checking for debounce function calls ===" rg "\bdebounce\b" app/ -g "!*.d.ts" # Search for debounceTask usage echo -e "\n=== Checking for debounceTask usage ===" rg "debounceTask" app/ -g "!*.d.ts" # Use ast-grep to find function calls echo -e "\n=== AST analysis of debounce calls ===" ast-grep --pattern 'debounce($$$)'Length of output: 4280
tests/integration/components/plus-n-list-test.js (1)
29-29: LGTM! Good modernization of array access.The change from
firstObjectto direct array indexing aligns with Ember's move towards native JavaScript patterns and away from prototype extensions. This change improves code clarity and performance.package.json (3)
209-213: Review the necessity of Storybook override.The override forcing ember-source version for @storybook/ember might indicate a compatibility issue. Consider:
- Checking if this override is still necessary with Storybook 8.4.6
- Verifying if there's an open issue or PR addressing this in the Storybook repository
✅ Verification successful
The Storybook override for ember-source is currently necessary
The override ensures @storybook/ember uses the same ember-source version (~5.12.0) as your project. This is required because Storybook doesn't officially support Ember 5 yet (see storybookjs/storybook#25385). Keep this override until official Ember 5 support is added to Storybook.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for related issues in Storybook's repo gh api graphql -f query=' { search(query: "repo:storybookjs/storybook ember-source", type: ISSUE, first: 5) { nodes { ... on Issue { title url state } } } }'Length of output: 495
Script:
#!/bin/bash # Check ember-source version in package.json jq '.dependencies["ember-source"] // .devDependencies["ember-source"]' package.jsonLength of output: 95
112-112: Verify compatibility between Ember core packages.The upgrade includes major version changes:
- ember-cli: ~5.12.0
- ember-data: ~5.3.8
- ember-source: ~5.12.0
Let's verify the compatibility between these versions.
Also applies to: 142-142, 163-163
✅ Verification successful
✅ Ember core packages versions are compatible
The versions specified in your package.json exactly match the officially supported versions from the ember-cli v5.12.0 blueprint:
- ember-cli: ~5.12.0
- ember-source: ~5.12.0
- ember-data: ~5.3.8
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any known compatibility issues between Ember packages curl -s https://raw.githubusercontent.com/ember-cli/ember-cli/v5.12.0/package.json | jq '.dependencies["ember-source"]' curl -s https://raw.githubusercontent.com/emberjs/data/v5.3.8/package.json | jq '.peerDependencies["ember-source"]'Length of output: 249
Script:
#!/bin/bash # Check ember-cli blueprint and release info curl -s https://raw.githubusercontent.com/ember-cli/ember-cli/v5.12.0/blueprints/app/files/package.json | jq '.devDependencies["ember-source"], .devDependencies["ember-data"]' curl -s https://raw.githubusercontent.com/ember-cli/ember-cli/v5.12.0/CHANGELOG.md | grep -A 5 "# 5.12.0"Length of output: 304
41-43: Verify Babel configuration for Ember 5.New Babel dependencies have been added:
- @babel/core
- @babel/eslint-parser
- @babel/plugin-proposal-decorators
These additions suggest changes in transpilation requirements for Ember 5. Please ensure that the Babel configuration in
.babelrcorember-cli-build.jsis updated accordingly.✅ Verification successful
Babel configuration is correctly set up for Ember 5
The current setup is already properly configured:
- ember-cli-babel v8.2.0 automatically handles decorator transformation for Ember 5
- No additional configuration is needed in ember-cli-build.js
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for Babel configuration fd -t f "babel.config.js|.babelrc|ember-cli-build.js" -x cat {}Length of output: 2504
Script:
#!/bin/bash # Search for other potential Babel configurations and ember addons managing decorators echo "=== Searching for other Babel configs ===" fd -t f ".babelrc.js|babel.config.js|.babelrc" -x cat {} echo -e "\n=== Checking package.json for ember-decorators or similar addons ===" cat package.json | grep -A 5 -B 5 "decorator" echo -e "\n=== Checking ember-cli-build.js for any decorator related config ===" cat ember-cli-build.js | grep -A 5 -B 5 "decorator"Length of output: 930
Script:
#!/bin/bash # Check for ember-cli-babel and decorator-related addons echo "=== Checking ember-cli-babel and related packages ===" cat package.json | grep -A 2 -B 2 "ember-cli-babel\|ember-decorator" echo -e "\n=== Checking ember addons that might configure decorators ===" cat package.json | grep -A 2 -B 2 "\"@ember\|\"ember-"Length of output: 4027
| 'stylelint-config-standard-scss', | ||
| 'stylelint-prettier/recommended', | ||
| ], | ||
| extends: ['stylelint-config-standard', 'stylelint-prettier/recommended'], |
There was a problem hiding this comment.
💡 Codebase verification
Restore stylelint-config-standard-scss to maintain SCSS linting capabilities
The project extensively uses SCSS with over 200 SCSS files across various components. Removing the SCSS stylelint configuration would impact the linting capabilities for these files.
🔗 Analysis chain
Verify SCSS usage before removing SCSS stylelint config.
The removal of stylelint-config-standard-scss might impact SCSS linting. Let's verify if the project still uses SCSS files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for SCSS files in the project
echo "Searching for SCSS files..."
fd -e scss
# Check for SCSS imports or references
echo -e "\nSearching for SCSS references in package.json..."
rg -i "scss" package.json
Length of output: 20823
| if (window.server) { | ||
| window.server.shutdown(); | ||
| } | ||
|
|
||
| // eslint-disable-next-line | ||
| server.shutdown(); |
There was a problem hiding this comment.
Remove duplicate server shutdown call.
There are two server.shutdown() calls:
- Inside the
if (window.server)block - Outside the block with an eslint-disable comment
This redundancy could cause issues if the server is already shut down.
if (window.server) {
window.server.shutdown();
}
- // eslint-disable-next-line
- server.shutdown();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (window.server) { | |
| window.server.shutdown(); | |
| } | |
| // eslint-disable-next-line | |
| server.shutdown(); | |
| if (window.server) { | |
| window.server.shutdown(); | |
| } |
| @@ -92,7 +92,7 @@ export default class PartnerClientReportDownloadComponent extends Component { | |||
| 'partner/partnerclient-report', | |||
| { | |||
| clientId: this.args.clientId, | |||
| id: this.reports.firstObject.id, | |||
| id: this.reports[0].id, | |||
There was a problem hiding this comment.
Add null check before accessing array index.
The code assumes this.reports is non-null, but this could lead to runtime errors. Consider adding a null check to match the pattern used earlier in the file.
- id: this.reports[0].id,
+ id: this.reports?.[0]?.id,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| id: this.reports[0].id, | |
| id: this.reports?.[0]?.id, |
| "@storybook/addon-actions": "8.4.6", | ||
| "@storybook/addon-docs": "8.4.6", | ||
| "@storybook/addon-essentials": "8.4.6", | ||
| "@storybook/addon-links": "8.4.6", | ||
| "@storybook/blocks": "8.4.6", | ||
| "@storybook/builder-webpack5": "8.4.6", | ||
| "@storybook/ember": "8.4.6", | ||
| "@storybook/ember-cli-storybook": "^0.6.1", | ||
| "@storybook/manager-webpack5": "6.5.16", |
There was a problem hiding this comment.
💡 Codebase verification
Update @storybook/manager-webpack5 to match other Storybook dependencies
The Storybook dependencies are at version 8.4.6, but @storybook/manager-webpack5 is at 6.5.16. This major version mismatch can cause compatibility issues. Additionally, no Storybook configuration files were found, which requires attention.
- Update @storybook/manager-webpack5 to version 8.4.6
- Verify Storybook configuration setup (no .storybook directory or config files found)
🔗 Analysis chain
Verify Storybook compatibility with Ember 5.
The Storybook upgrade to 8.4.6 is significant. The override for ember-source in the Storybook configuration suggests potential compatibility concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Storybook configuration and preview files
fd -t f "\.story(book|ies)\." -x cat {}
Length of output: 43
Script:
#!/bin/bash
# Check for Storybook config files and Ember version
echo "=== Storybook Config Files ==="
fd -t f "^(main|preview)\.(js|ts)$" -x echo "=== {} ===" \; -x cat {}
echo -e "\n=== Package.json Dependencies ==="
cat package.json | jq '.dependencies["ember-source"], .devDependencies["ember-source"]'
echo -e "\n=== Storybook Config Locations ==="
fd -t d "\.storybook$"
Length of output: 443
SmitGala
force-pushed
the
ember-upgrade-v5
branch
from
58c9cf9 to
b0eb293
Compare
Deploying irenestaging with
|
| Latest commit: |
5b6dcf0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://017c02fb.irenestaging.pages.dev |
| Branch Preview URL: | https://ember-upgrade-v5.irenestaging.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
app/components/partner/client-uploads-stat-chart/index.js (2)
83-90: Consider modernizing array access while maintaining constructor implementation.The constructor implementation looks good, properly extending dayjs plugins and initializing properties. However, consider modernizing the array access:
Replace the deprecated
objectAt(0)with modern array access:- this.currentTimeline = this.timelinePlaceholders.objectAt(0); + this.currentTimeline = this.timelinePlaceholders[0];This aligns with Ember's deprecation of array prototype extensions and the v5 upgrade goals.
Line range hint
92-150: Modernize remaining array extension usages.For consistency with Ember v5 and to remove dependency on deprecated array prototype extensions, consider updating the remaining
objectAtusages in the getters:get startDate() { - return this.dateRange.objectAt(0); + return this.dateRange[0]; } get endDate() { - return this.dateRange.objectAt(1); + return this.dateRange[1]; }tests/unit/models/file-test.js (4)
14-14: Use a more descriptive test name.The test name "it passes" doesn't describe what functionality is being tested. Consider renaming to something more specific like "file model correctly handles status transitions and flags".
19-41: Use assert.strictEqual and assert.true/false for boolean assertions.For better readability and type safety, consider:
- Using
assert.true()andassert.false()for boolean checks- Using
assert.strictEqual()instead ofassert.equal()for explicit type checkingExample refactor:
-assert.equal(file.get('isManualRequested'), true, 'Manual Requested'); +assert.true(file.get('isManualRequested'), 'Manual Requested'); -assert.equal(file.get('isRunningApiScan'), false, 'API Scan'); +assert.false(file.get('isRunningApiScan'), 'API Scan');
43-68: Consider grouping related status transitions.The status transition tests could be more maintainable by grouping related states and removing unnecessary empty lines. Consider using QUnit's test grouping or creating separate test cases for different status categories.
Example refactor:
test('status text reflects dynamic status changes', function (assert) { const statusChecks = [ { status: ENUMS.DYNAMIC_STATUS.BOOTING, expected: 'Booting' }, { status: ENUMS.DYNAMIC_STATUS.DOWNLOADING, expected: 'Downloading' }, // ... other status checks ]; const file = this.owner.lookup('service:store').createRecord('file', { id: 1 }); statusChecks.forEach(({ status, expected }) => { file.set('dynamicStatus', status); assert.strictEqual(file.get('statusText'), expected, `Status text is '${expected}' when status is ${status}`); }); });
Line range hint
1-1: Consider addressing eslint issues instead of disabling them.Rather than disabling multiple eslint rules, consider:
- Adding an expect count to satisfy
qunit/require-expect- Using appropriate assert methods to satisfy
qunit/no-assert-equalandqunit/no-assert-equal-boolean- Fixing formatting to satisfy
prettier/prettier
📜 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)
.stylelintrc.js(1 hunks)app/adapters/jira-repo.ts(1 hunks)app/components/ak-drawer/index.ts(3 hunks)app/components/ak-select/before-option/index.ts(2 hunks)app/components/ak-select/index.ts(2 hunks)app/components/ak-tooltip/index.ts(4 hunks)app/components/analysis-risk/override-edit-drawer/index.ts(2 hunks)app/components/app-monitoring/settings/index.ts(2 hunks)app/components/appknox-wrapper/onboarding-guide/index.ts(2 hunks)app/components/file-compare/table/index.ts(2 hunks)app/components/file-details/dynamic-scan/action/expiry/index.ts(2 hunks)app/components/github-account/index.js(1 hunks)app/components/organization-member/list/add-to-team/index.ts(2 hunks)app/components/organization-member/list/index.ts(2 hunks)app/components/organization-member/list/member-details/index.ts(2 hunks)app/components/organization-team/add-team-member/index.ts(2 hunks)app/components/organization-team/add-team-project/index.ts(2 hunks)app/components/organization-team/index.ts(2 hunks)app/components/organization-team/invitation-list/index.ts(2 hunks)app/components/organization-team/member-list/index.ts(2 hunks)app/components/organization-team/project-list/index.ts(2 hunks)app/components/organization/service-account/add-project/list/index.ts(2 hunks)app/components/organization/service-account/create/index.ts(2 hunks)app/components/organization/service-account/section/access-token/index.ts(2 hunks)app/components/organization/service-account/section/select-project/list/index.ts(2 hunks)app/components/partner/client-report-download/index.js(1 hunks)app/components/partner/client-uploads-stat-chart/index.js(1 hunks)app/components/project-settings/general-settings/add-project-collaborator/table/index.ts(2 hunks)app/components/project-settings/general-settings/add-project-team/table/index.ts(2 hunks)app/components/project-settings/general-settings/jira-project/index.ts(0 hunks)app/components/sbom/app-list/index.ts(2 hunks)app/components/sbom/scan-details/component-list/index.ts(2 hunks)app/components/security/project-search-list/index.ts(2 hunks)app/components/storeknox/discover/requested-apps/index.ts(1 hunks)app/components/system-status/index.ts(2 hunks)app/routes/authenticated/dashboard/app-monitoring/monitoring-details.ts(1 hunks)app/routes/authenticated/dashboard/app-monitoring/monitoring-details/history.ts(2 hunks)app/routes/authenticated/dashboard/app-monitoring/monitoring-details/index.ts(2 hunks)app/services/body-class.js(2 hunks)app/services/websocket.ts(2 hunks)config/environment.js(1 hunks)lib/ember-django-adapter/package.json(1 hunks)package.json(6 hunks)tests/acceptance/file-compare-test.js(1 hunks)tests/helpers/destroy-app.js(1 hunks)tests/helpers/start-app.js(1 hunks)tests/integration/components/file-details/vulnerability-analysis-details/index-test.js(14 hunks)tests/integration/components/plus-n-list-test.js(1 hunks)tests/unit/initializers/irene-test.js(1 hunks)tests/unit/models/analysis-test.js(1 hunks)tests/unit/models/analytics/recent-issue-test.js(1 hunks)tests/unit/models/api-scan-options-test.js(1 hunks)tests/unit/models/attachment-test.js(1 hunks)tests/unit/models/available-device-test.js(1 hunks)tests/unit/models/capturedapi-test.js(1 hunks)tests/unit/models/coupon-test.js(1 hunks)tests/unit/models/device-preference-test.js(1 hunks)tests/unit/models/device-test.js(1 hunks)tests/unit/models/file-test.js(1 hunks)tests/unit/models/invoice-test.js(1 hunks)tests/unit/models/manualscan-test.js(1 hunks)tests/unit/models/mfa-test.js(1 hunks)tests/unit/models/organization-mfa-test.js(1 hunks)tests/unit/models/organization-namespace-test.js(1 hunks)tests/unit/models/organization-user-test.js(1 hunks)tests/unit/models/owasp-test.js(1 hunks)tests/unit/models/owaspmobile2024-test.js(1 hunks)tests/unit/models/personaltoken-test.js(1 hunks)tests/unit/models/plan-test.js(1 hunks)tests/unit/models/pricing-test.js(1 hunks)tests/unit/models/profile-test.js(1 hunks)tests/unit/models/project-team-test.js(1 hunks)tests/unit/models/project-test.js(1 hunks)tests/unit/models/proxy-settings-test.js(1 hunks)tests/unit/models/security/analysis-test.js(1 hunks)tests/unit/models/security/attachment-test.js(1 hunks)tests/unit/models/security/file-test.js(1 hunks)tests/unit/models/security/project-test.js(1 hunks)tests/unit/models/security/vulnerability-test.js(1 hunks)tests/unit/models/submission-test.js(1 hunks)tests/unit/models/subscription-test.js(1 hunks)tests/unit/models/tag-test.js(1 hunks)
⛔ Files not processed due to max files limit (9)
- tests/unit/models/team-invitation-test.js
- tests/unit/models/team-member-test.js
- tests/unit/models/team-project-test.js
- tests/unit/models/unknown-analysis-status-test.js
- tests/unit/models/user-test.js
- tests/unit/models/vulnerability-preference-test.js
- tests/unit/models/vulnerability-test.js
- tests/unit/routes/authenticated/security/file-test.js
- tests/unit/serializers/mfa-test.js
💤 Files with no reviewable changes (1)
- app/components/project-settings/general-settings/jira-project/index.ts
🚧 Files skipped from review as they are similar to previous changes (77)
- .stylelintrc.js
- tests/unit/models/organization-namespace-test.js
- app/components/sbom/scan-details/component-list/index.ts
- tests/unit/initializers/irene-test.js
- tests/unit/models/submission-test.js
- app/components/partner/client-report-download/index.js
- app/routes/authenticated/dashboard/app-monitoring/monitoring-details.ts
- tests/unit/models/project-team-test.js
- app/components/file-compare/table/index.ts
- tests/unit/models/coupon-test.js
- tests/unit/models/available-device-test.js
- app/components/security/project-search-list/index.ts
- config/environment.js
- app/components/ak-select/before-option/index.ts
- tests/unit/models/security/project-test.js
- tests/unit/models/security/file-test.js
- app/components/app-monitoring/settings/index.ts
- app/components/appknox-wrapper/onboarding-guide/index.ts
- tests/integration/components/plus-n-list-test.js
- app/components/organization-member/list/member-details/index.ts
- tests/unit/models/device-test.js
- app/components/organization/service-account/section/access-token/index.ts
- tests/unit/models/organization-mfa-test.js
- tests/unit/models/plan-test.js
- tests/unit/models/api-scan-options-test.js
- tests/unit/models/security/analysis-test.js
- app/routes/authenticated/dashboard/app-monitoring/monitoring-details/index.ts
- tests/unit/models/proxy-settings-test.js
- app/components/organization-member/list/index.ts
- app/components/organization-team/index.ts
- tests/unit/models/security/vulnerability-test.js
- tests/unit/models/capturedapi-test.js
- tests/unit/models/personaltoken-test.js
- lib/ember-django-adapter/package.json
- app/components/organization-member/list/add-to-team/index.ts
- tests/helpers/destroy-app.js
- app/components/system-status/index.ts
- app/components/organization-team/invitation-list/index.ts
- tests/unit/models/pricing-test.js
- tests/unit/models/device-preference-test.js
- app/components/organization-team/add-team-member/index.ts
- app/components/organization-team/project-list/index.ts
- tests/unit/models/project-test.js
- app/components/ak-select/index.ts
- app/components/ak-drawer/index.ts
- tests/unit/models/attachment-test.js
- tests/unit/models/organization-user-test.js
- app/services/body-class.js
- tests/unit/models/analysis-test.js
- tests/unit/models/owasp-test.js
- app/routes/authenticated/dashboard/app-monitoring/monitoring-details/history.ts
- tests/unit/models/analytics/recent-issue-test.js
- app/components/project-settings/general-settings/add-project-collaborator/table/index.ts
- tests/unit/models/tag-test.js
- tests/unit/models/owaspmobile2024-test.js
- app/components/organization/service-account/add-project/list/index.ts
- app/components/organization-team/add-team-project/index.ts
- tests/unit/models/profile-test.js
- app/components/organization/service-account/section/select-project/list/index.ts
- app/components/analysis-risk/override-edit-drawer/index.ts
- app/components/sbom/app-list/index.ts
- tests/acceptance/file-compare-test.js
- app/components/organization-team/member-list/index.ts
- app/components/ak-tooltip/index.ts
- tests/unit/models/security/attachment-test.js
- tests/unit/models/manualscan-test.js
- tests/helpers/start-app.js
- tests/unit/models/mfa-test.js
- tests/unit/models/invoice-test.js
- app/components/github-account/index.js
- app/components/project-settings/general-settings/add-project-team/table/index.ts
- app/components/organization/service-account/create/index.ts
- app/components/storeknox/discover/requested-apps/index.ts
- app/adapters/jira-repo.ts
- tests/integration/components/file-details/vulnerability-analysis-details/index-test.js
- tests/unit/models/subscription-test.js
- app/components/file-details/dynamic-scan/action/expiry/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Run Integration Tests - Part 8
- GitHub Check: Run Integration Tests - Part 7
- GitHub Check: Run Integration Tests - Part 6
- GitHub Check: Run Integration Tests - Part 5
- GitHub Check: Run Integration Tests - Part 4
- GitHub Check: Run Integration Tests - Part 3
- GitHub Check: Run Integration Tests - Part 2
- GitHub Check: Run Integration Tests - Part 1
- GitHub Check: Run Linting
- GitHub Check: Cloudflare Pages
🔇 Additional comments (11)
app/components/partner/client-uploads-stat-chart/index.js (1)
75-75: LGTM! Good practice moving initialization to constructor.Moving the
currentTimelineinitialization from the property declaration to the constructor follows modern Ember practices and ensures proper initialization order.tests/unit/models/file-test.js (2)
6-13: LGTM! Clean test setup.The module setup follows modern QUnit best practices with proper test environment initialization.
69-75: Fix incorrect assertion messages.The assertion messages for status setter methods are still incorrect, using "Set Booting Status" for all cases.
This issue was previously identified in a past review. Please apply the suggested fix:
assert.equal(file.setBootingStatus(), undefined, 'Set Booting Status'); -assert.equal(file.setShuttingDown(), undefined, 'Set Booting Status'); -assert.equal(file.setNone(), undefined, 'Set Booting Status'); -assert.equal(file.setReady(), undefined, 'Set Booting Status'); +assert.equal(file.setShuttingDown(), undefined, 'Set Shutting Down Status'); +assert.equal(file.setNone(), undefined, 'Set None Status'); +assert.equal(file.setReady(), undefined, 'Set Ready Status');package.json (6)
112-112: Core Ember upgrades look good!The upgrade to Ember 5 has been done correctly with compatible versions for core packages:
- ember-cli: ~5.12.0
- ember-data: ~5.3.8
- ember-qunit: ^8.1.0
- ember-source: ~5.12.0
Also applies to: 142-142, 159-160, 163-163
58-66: Update @storybook/manager-webpack5 to match other Storybook dependenciesThe Storybook dependencies are at version 8.4.6, but @storybook/manager-webpack5 is at 6.5.16. This major version mismatch can cause compatibility issues.
50-50: Testing framework updates look good!The testing dependencies have been updated to compatible versions:
- @ember/test-helpers: ^3.3.1
- qunit: ^2.22.0
- qunit-dom: ^3.2.1
Also applies to: 182-183
169-173: Linting tool updates look good!The linting dependencies have been updated to compatible versions:
- eslint: ^8.57.1
- eslint-plugin-ember: ^12.2.1
- eslint-plugin-prettier: ^5.2.1
- stylelint: ^15.11.0
- stylelint-config-standard: ^34.0.0
Also applies to: 187-189
214-216: Node.js configuration looks good!The Node.js version requirement (>=20) is correctly specified and aligns with Ember 5's requirements. The Volta configuration ensures consistent Node.js version (^20.18.0) across the team.
Also applies to: 221-224
41-43: Verify Babel dependency versionsPlease verify that these Babel versions exist and are stable:
- @babel/core: ^7.25.2
- @babel/eslint-parser: ^7.25.1
- @babel/plugin-proposal-decorators: ^7.24.7
✅ Verification successful
Babel dependency versions are valid
All specified Babel package versions exist in the npm registry and are valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if these Babel versions exist in npm registry echo "=== Checking @babel/core ===" curl -s https://registry.npmjs.org/@babel/core | jq '.versions | keys[]' | grep "7.25.2" echo -e "\n=== Checking @babel/eslint-parser ===" curl -s https://registry.npmjs.org/@babel/eslint-parser | jq '.versions | keys[]' | grep "7.25.1" echo -e "\n=== Checking @babel/plugin-proposal-decorators ===" curl -s https://registry.npmjs.org/@babel/plugin-proposal-decorators | jq '.versions | keys[]' | grep "7.24.7"Length of output: 599
app/services/websocket.ts (2)
5-5: LGTM!The import of
debounceTaskfromember-lifelineis correctly placed and follows TypeScript conventions.
238-238: LGTM! Verify complete migration from runloop.The change to
debounceTaskfollows ember-lifeline patterns correctly and maintains the same debouncing behavior.Let's verify that all
@ember/runloopimports have been removed from this file:✅ Verification successful
Migration to ember-lifeline verified successfully
The file has been fully migrated from
@ember/runlooptoember-lifelinewith no remaining runloop imports or method usages.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining @ember/runloop imports rg "@ember/runloop" "app/services/websocket.ts" # Also check for any remaining runloop method usage ast-grep --pattern 'debounce($$$)' "app/services/websocket.ts"Length of output: 108
Script:
#!/bin/bash # Check for any runloop imports with various syntax rg -i "from ['\"]\@ember/runloop" "app/services/websocket.ts" # Check for ember-lifeline import rg "from ['\"](ember-lifeline|@ember/destroyable)" "app/services/websocket.ts" # Check for any remaining runloop method usage ast-grep --pattern 'run($$$)' ast-grep --pattern 'later($$$)' ast-grep --pattern 'schedule($$$)' ast-grep --pattern 'scheduleOnce($$$)' ast-grep --pattern 'throttle($$$)' ast-grep --pattern 'next($$$)'Length of output: 505
Script:
#!/bin/bash # Check for any @ember/runloop imports with simpler pattern rg "@ember/runloop" "app/services/websocket.ts"Length of output: 45
SmitGala
force-pushed
the
ember-upgrade-v5
branch
from
b0eb293 to
5b6dcf0
Compare
|
No description provided.