UseEffect Resource Implementation #1578
Conversation
WalkthroughThis pull request systematically refactors Ember.js components to replace Ember observers with a new reactive Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying irenestaging with
|
| Latest commit: |
df22ed5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://74d4b3bb.irenestaging.pages.dev |
| Branch Preview URL: | https://pd-1845-poc-implement-effect.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: 3
🧹 Nitpick comments (5)
app/components/ak-tree/provider/index.hbs (1)
1-3: Clean implementation with minor comment improvement needed.The effect invocation follows the established pattern and the
initializeEffectname is descriptive. However, the comment "Initialize initialize effect" is redundant.Consider improving the comment for clarity:
-{{! Initialize initialize effect }} +{{! Initialize effect }}app/helpers/use-effect.ts (1)
25-25: Replacevoidwithundefinedin the union type.Using
voidin a union type is confusing and not idiomatic TypeScript.-export type EffectReturnType<T> = void | EffectCleanupObject<T>; +export type EffectReturnType<T> = undefined | EffectCleanupObject<T>;app/components/organization-invitation-list/index.ts (2)
73-73: Remove unused tracked property.The
querytracked property is defined but never used in the component.- @tracked query = '';
221-221: Use optional chaining for safer property access.- errMsg = err.errors[0]?.detail || errMsg; + errMsg = err.errors?.[0]?.detail || errMsg;app/services/project.ts (1)
73-73: Use optional chaining for safer property access.- errMsg = err.errors[0]?.detail || errMsg; + errMsg = err.errors?.[0]?.detail || errMsg;
📜 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 (48)
app/components/ak-chart/index.hbs(1 hunks)app/components/ak-chart/index.ts(2 hunks)app/components/ak-popover/index.hbs(2 hunks)app/components/ak-popover/index.ts(3 hunks)app/components/ak-tree/provider/index.hbs(1 hunks)app/components/ak-tree/provider/index.ts(2 hunks)app/components/file-details/dynamic-scan/scheduled-automated/index.hbs(1 hunks)app/components/file-details/dynamic-scan/scheduled-automated/index.ts(3 hunks)app/components/file-list/index.hbs(1 hunks)app/components/file-list/index.ts(5 hunks)app/components/file/report-drawer/privacy-reports/item/index.hbs(1 hunks)app/components/file/report-drawer/privacy-reports/item/index.ts(3 hunks)app/components/file/report-drawer/va-reports/index.hbs(1 hunks)app/components/file/report-drawer/va-reports/index.ts(3 hunks)app/components/organization-invitation-list/index.hbs(2 hunks)app/components/organization-invitation-list/index.ts(5 hunks)app/components/partner/invitation-list/index.hbs(1 hunks)app/components/partner/invitation-list/index.ts(3 hunks)app/components/partner/registration-request-pending-list/index.hbs(1 hunks)app/components/partner/registration-request-pending-list/index.ts(3 hunks)app/components/partner/registration-request-rejected-list/index.hbs(1 hunks)app/components/partner/registration-request-rejected-list/index.ts(2 hunks)app/components/project-list/index.ts(2 hunks)app/components/project-settings/general-settings/collaborators-table/index.hbs(1 hunks)app/components/project-settings/general-settings/collaborators-table/index.ts(2 hunks)app/components/project-settings/general-settings/project-team-table/index.hbs(1 hunks)app/components/project-settings/general-settings/project-team-table/index.ts(2 hunks)app/components/sbom/scan-report-drawer/report-list/index.hbs(1 hunks)app/components/sbom/scan-report-drawer/report-list/index.ts(3 hunks)app/components/security/file-search-list/index.ts(0 hunks)app/components/security/project-search-list/index.ts(0 hunks)app/components/storeknox/inventory-details/app-details/va-results/index.hbs(1 hunks)app/components/storeknox/inventory-details/app-details/va-results/index.ts(5 hunks)app/components/storeknox/inventory-details/unscanned-version/table/actions/index.hbs(1 hunks)app/components/storeknox/inventory-details/unscanned-version/table/actions/index.ts(4 hunks)app/components/upload-app/status/details/index.hbs(1 hunks)app/components/upload-app/status/details/index.ts(2 hunks)app/components/upload-app/status/index.hbs(1 hunks)app/components/upload-app/status/index.ts(4 hunks)app/helpers/use-effect.ts(1 hunks)app/services/project.ts(1 hunks)package.json(1 hunks)tests/acceptance/deprecated-routes/account-settings-redirect-test.js(1 hunks)tests/acceptance/storeknox/inventory-details/store-verson-tables-test.js(1 hunks)tests/integration/components/partner/invitation-list-test.js(0 hunks)tests/integration/components/partner/registration-request-pending-list-test.js(0 hunks)tests/integration/components/sbom/scan-report-drawer-test.js(0 hunks)tests/unit/helpers/use-effect-test.js(1 hunks)
💤 Files with no reviewable changes (5)
- app/components/security/project-search-list/index.ts
- app/components/security/file-search-list/index.ts
- tests/integration/components/sbom/scan-report-drawer-test.js
- tests/integration/components/partner/invitation-list-test.js
- tests/integration/components/partner/registration-request-pending-list-test.js
🧰 Additional context used
🧬 Code Graph Analysis (17)
app/components/ak-chart/index.ts (1)
app/helpers/use-effect.ts (1)
useEffect(117-119)
app/components/project-list/index.ts (1)
tests/integration/components/project-list-test.js (3)
platform(328-328)platform(424-424)team(150-150)
app/components/ak-tree/provider/index.ts (1)
app/helpers/use-effect.ts (1)
useEffect(117-119)
app/components/upload-app/status/index.ts (1)
app/helpers/use-effect.ts (1)
useEffect(117-119)
app/components/project-settings/general-settings/project-team-table/index.ts (1)
app/helpers/use-effect.ts (1)
useEffect(117-119)
app/components/partner/registration-request-rejected-list/index.ts (1)
app/helpers/use-effect.ts (1)
useEffect(117-119)
app/components/upload-app/status/details/index.ts (1)
app/helpers/use-effect.ts (1)
useEffect(117-119)
app/components/file/report-drawer/privacy-reports/item/index.ts (1)
app/helpers/use-effect.ts (1)
useEffect(117-119)
app/components/ak-popover/index.ts (1)
app/helpers/use-effect.ts (1)
useEffect(117-119)
app/components/project-settings/general-settings/collaborators-table/index.ts (1)
app/helpers/use-effect.ts (1)
useEffect(117-119)
app/components/file/report-drawer/va-reports/index.ts (1)
app/helpers/use-effect.ts (1)
useEffect(117-119)
app/components/file-details/dynamic-scan/scheduled-automated/index.ts (1)
app/helpers/use-effect.ts (1)
useEffect(117-119)
app/components/partner/invitation-list/index.ts (1)
app/helpers/use-effect.ts (1)
useEffect(117-119)
app/components/partner/registration-request-pending-list/index.ts (1)
app/helpers/use-effect.ts (1)
useEffect(117-119)
app/components/organization-invitation-list/index.ts (3)
app/helpers/use-effect.ts (1)
useEffect(117-119)app/components/ak-pagination-provider/index.ts (1)
PaginationProviderActionsArgs(15-15)types/irene/index.d.ts (1)
AdapterError(146-154)
app/components/storeknox/inventory-details/unscanned-version/table/actions/index.ts (1)
app/helpers/use-effect.ts (1)
useEffect(117-119)
app/components/storeknox/inventory-details/app-details/va-results/index.ts (1)
app/helpers/use-effect.ts (1)
useEffect(117-119)
🪛 Biome (1.9.4)
app/helpers/use-effect.ts
[error] 25-25: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
app/components/organization-invitation-list/index.ts
[error] 221-221: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/services/project.ts
[error] 73-73: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Run Integration Tests - Part 5
- GitHub Check: Run Acceptance Tests
- GitHub Check: Run Unit Tests
- GitHub Check: Run Linting
- GitHub Check: Run E2E Tests (5)
- GitHub Check: Run E2E Tests (4)
- GitHub Check: Run E2E Tests (3)
- GitHub Check: Run E2E Tests (1)
- GitHub Check: Run E2E Tests (2)
- GitHub Check: Setup & Build Application
- GitHub Check: Publish to Cloudflare Pages
🔇 Additional comments (68)
app/components/file/report-drawer/privacy-reports/item/index.hbs (1)
1-2: LGTM! Effect initialization follows the new pattern.The effect invocation at the template level aligns with the systematic refactor to replace observers with
useEffect. The clear comment and positioning before conditional rendering is appropriate.app/components/upload-app/status/index.hbs (1)
1-2: LGTM! Consistent with the effect pattern.The submission reload effect is properly initialized at the template level, following the same pattern established across the refactor.
app/components/storeknox/inventory-details/app-details/va-results/index.hbs (1)
1-2: LGTM! Effect properly documented and positioned.The SK app reload effect follows the established pattern with a clear, descriptive comment explaining when it triggers.
app/components/project-settings/general-settings/collaborators-table/index.hbs (1)
1-2: LGTM! Effect initialization is consistent.The project collaborator counter effect follows the established pattern. The effect name is self-explanatory even with the brief comment.
tests/acceptance/storeknox/inventory-details/store-verson-tables-test.js (1)
43-45: LGTM! Server mock follows established patterns.The new route handler for individual SK app versions correctly returns a single record by ID, consistent with existing mock implementations.
app/components/upload-app/status/details/index.hbs (1)
1-3: LGTM! Clean implementation of the useEffect pattern.The effect invocation follows the consistent pattern established across the codebase with clear comments and proper structure.
app/components/partner/registration-request-pending-list/index.hbs (1)
1-4: Consistent implementation of the effect pattern.The template correctly follows the established pattern with proper commenting structure and effect invocation placement.
app/components/partner/invitation-list/index.hbs (1)
1-4: Consistent effect pattern implementation.The template structure matches the established pattern and aligns with the similar partner component for registration request counter tracking.
app/components/file-details/dynamic-scan/scheduled-automated/index.hbs (1)
1-4: Excellent use of descriptive comments for effect initialization.The specific comment clearly explains the effect's purpose, which enhances code maintainability while following the established pattern.
app/components/file-list/index.hbs (1)
1-4: Well-implemented effect initialization with clear documentation.The descriptive comment and consistent pattern implementation effectively support the transition from observer-based to effect-based reactive updates.
app/components/partner/registration-request-rejected-list/index.hbs (1)
1-4: Good implementation of the useEffect pattern.The template correctly implements the new useEffect pattern by invoking
{{this.registrationRequestCounterEffect}}at the top. The commenting structure clearly separates the effect list from the component content, which improves readability and maintainability.app/components/ak-chart/index.hbs (1)
1-2: Consistent useEffect pattern implementation.The template correctly implements the useEffect pattern by invoking the
chartOptionChangeEffectat the top of the template, aligning with the broader refactor to replace observers with reactive effects.app/components/file/report-drawer/va-reports/index.hbs (1)
1-4: Well-documented useEffect implementation.The template correctly implements the useEffect pattern with a clear, descriptive comment explaining the purpose of the effect. This improves code readability and helps other developers understand the reactive behavior.
app/components/storeknox/inventory-details/unscanned-version/table/actions/index.hbs (1)
1-4: Excellent useEffect implementation with clear documentation.The template correctly implements the useEffect pattern with a specific, descriptive comment that clearly explains the reactive behavior. This level of documentation makes the code more maintainable and helps developers understand the effect's purpose.
app/components/sbom/scan-report-drawer/report-list/index.hbs (1)
1-3: Clean implementation of useEffect pattern.The effect invocation follows the established pattern with clear commenting structure and descriptive naming. The
reloadLatestSbomScanReportEffectname accurately reflects its purpose of reloading the latest SBOM scan report.app/components/project-settings/general-settings/project-team-table/index.hbs (1)
1-3: Consistent implementation of the useEffect pattern.The effect invocation follows the established pattern with clear separation between effects and component markup. The
projectTeamCounterEffectname is descriptive and aligns with the component's functionality.app/components/organization-invitation-list/index.hbs (2)
1-3: Proper implementation of the useEffect pattern.The effect invocation follows the established pattern with clear commenting and descriptive naming. The
invitationCounterEffectname clearly indicates its purpose.
46-46: Verify the action method refactoring.The change from
this.handleReloadInvitestothis.reloadInvitessuggests a refactoring of the action method. Ensure that the new method name is consistent with the component's TypeScript implementation and that all references have been updated.#!/bin/bash # Description: Verify the action method name change is consistent across the codebase # Expected: Find references to the new method name and ensure no old references remain # Check for the new method name in the component TypeScript file fd -e ts -e js | xargs rg -l "organization-invitation-list" | xargs rg -C 3 "reloadInvites" # Check for any remaining references to the old method name rg -C 3 "handleReloadInvites"app/components/ak-popover/index.hbs (1)
1-3: Proper implementation of the useEffect pattern.The effect invocation follows the established pattern with clear commenting structure. The
toggleMountContentEffectname is descriptive and clearly indicates its purpose for managing popover content mounting.package.json (1)
200-200: Verify dependency versions and placement.The new dependencies support the
useEffecthelper implementation. Please ensure:
- These versions are secure and up-to-date
@types/lodashshould typically be indevDependenciessince it's only needed during development/build time- Consider if
lodashandember-resourcesshould be indevDependenciesif they're only used by helper functions#!/bin/bash # Description: Check for latest versions and security advisories for the new dependencies echo "Checking lodash..." curl -s https://pypi.org/pypi/lodash/json | jq '.info.version' 2>/dev/null || echo "lodash is not a Python package" npm view lodash version echo "Checking ember-resources..." npm view ember-resources version echo "Checking @types/lodash..." npm view @types/lodash version # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: NPM, package: "lodash") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Also applies to: 205-205, 208-208
app/components/ak-chart/index.ts (2)
39-39: LGTM!Clean import of the new
useEffecthelper.
84-89: Excellent refactor from observers to declarative effects!The
useEffectimplementation properly:
- Watches the
optionargument as a dependency- Triggers the existing
handleOptionChangemethod when changes occur- Replaces imperative observer management with declarative reactive effects
This is a clean example of the new pattern being applied consistently.
app/components/project-list/index.ts (1)
92-101: Consistent API refactor aligns with service changes.The refactor consistently updates all project fetching calls to use the new two-step pattern:
setQueryParams({...})- centralizes query parameter settingfetchProjects.perform()- executes the fetch without parametersThis pattern is applied uniformly across all actions and maintains the same functionality while centralizing query parameter management.
Verify that the project service properly implements the new
setQueryParamsmethod:#!/bin/bash # Description: Verify the project service has the new setQueryParams method # Check if setQueryParams method exists in project service ast-grep --pattern $'setQueryParams($$$) { $$$ }' # Also check for the updated fetchProjects task signature rg -A 10 "fetchProjects.*task" --type tsAlso applies to: 109-118, 125-134, 141-150, 157-166, 172-181, 190-199
app/components/project-settings/general-settings/project-team-table/index.ts (2)
19-19: LGTM!Clean import of the
useEffecthelper for reactive effects.
43-48: Excellent refactor from manual observer management to declarative effects!The
useEffectimplementation:
- Watches
realtime.ProjectTeamCounterfor changes- Triggers
reloadProjectTeamListwhen the counter updates- Eliminates the need for manual observer registration/removal in constructor/willDestroy
This simplifies the component lifecycle and makes reactive behavior more declarative.
app/components/partner/registration-request-rejected-list/index.ts (4)
7-8: Improved TypeScript imports!Good practice using
typeimports for interfaces and types that are only used for type annotations. This helps with tree-shaking and makes the import intentions clearer.Also applies to: 11-11, 15-16
13-13: LGTM!Clean import of the
useEffecthelper.
37-43: Clean refactor from observers to reactive effects!The
useEffectimplementation properly:
- Watches
realtime.RegistrationRequestCounterfor changes- Triggers the
reloadRegistrationRequestaction when the counter updates- Replaces manual observer lifecycle management with declarative reactive behavior
51-61: Good conversion of callback to action method.The former observer callback is now properly converted to an action method that directly calls the fetch task. The logic remains the same while fitting better with the new reactive pattern.
app/components/ak-tree/provider/index.ts (2)
4-4: LGTM! Clean import addition for useEffect helper.The import statement properly brings in the useEffect helper from the correct module path.
84-90: Excellent refactor from observer pattern to reactive effects.This change successfully replaces manual observer lifecycle management with a declarative reactive effect. The configuration correctly:
- Specifies the
initializemethod as the effect callback- Tracks
this.args.treeDataas a dependency- Eliminates the need for manual observer setup/teardown
The effect will automatically re-run whenever
treeDatachanges, providing the same reactive behavior as the previous observer pattern but with cleaner lifecycle management.app/components/ak-popover/index.ts (3)
13-13: LGTM! Proper import of useEffect helper.The import statement correctly brings in the useEffect helper for replacing the observer pattern.
66-72: Great refactor to reactive effects pattern.This change successfully replaces the manual observer setup for
anchorRefchanges with a declarative reactive effect. The effect configuration properly:
- Uses
toggleMountContentas the effect callback- Tracks
this.args.anchorRefas a dependency- Eliminates manual observer lifecycle management
This provides cleaner, more maintainable reactive behavior.
152-166: Improved logic flow in toggleMountContent method.The refactored method logic properly handles different states:
- When
anchorRefexists: creates popper instance and mounts content- When popper exists but no
anchorRef: destroys popper and conditionally unmounts based onunmountOnClose- When no popper exists: sets mount state based on
mountOnOpenflagThe conditional logic is clear and maintains the expected behavior while working well with the new reactive effect system.
app/components/upload-app/status/details/index.ts (2)
5-16: Good import organization and useEffect integration.The import statements are well-organized with type imports properly separated from runtime imports. The useEffect import is correctly added for the observer pattern refactor.
36-41: Solid refactor to reactive effect pattern.This change successfully replaces the manual observer for
submission.statuschanges with a declarative reactive effect. The configuration correctly:
- Specifies
refreshProjectListas the effect callback- Tracks
this.args.submission.statusas the dependency- Eliminates manual observer lifecycle management
The effect will automatically trigger project list refresh when the submission status changes, maintaining the same reactive behavior with cleaner code.
app/components/file/report-drawer/va-reports/index.ts (3)
15-15: LGTM! Correct useEffect import.The import statement properly brings in the useEffect helper for the observer pattern refactor.
59-65: Excellent migration to reactive effects.This change successfully replaces the manual observer for
ReportCounterchanges with a declarative reactive effect. The configuration properly:
- Uses
reloadReportsas the effect callback- Tracks
this.realtime.ReportCounteras a dependency- Eliminates the need for manual observer setup and cleanup
This provides the same reactive behavior with much cleaner lifecycle management.
212-214: Properly structured action method.The
reloadReportsmethod is correctly decorated with@actionand properly calls thegetReportstask. The method name clearly describes its purpose and integrates well with the new reactive effect system.app/components/upload-app/status/index.ts (4)
20-20: LGTM! Proper useEffect import.The import statement correctly brings in the useEffect helper for replacing the observer pattern.
52-58: Excellent observer-to-effect refactor.This change successfully replaces manual observer management for
SubmissionCounterchanges with a declarative reactive effect. The configuration correctly:
- Uses
reloadSubmissionsas the effect callback- Tracks
this.realtime.SubmissionCounteras a dependency- Eliminates manual observer lifecycle management
This provides cleaner, more maintainable reactive behavior while preserving the same functionality.
98-110: Good type safety improvement with const assertions.Adding
as constassertions to the icon name literals ('downloading','download-done','error') ensures these values are treated as literal types rather than general string types. This provides better type safety and autocompletion support.
191-193: Well-structured action method.The
reloadSubmissionsmethod is properly decorated with@actionand correctly calls thegetSubmissionstask. The method integrates seamlessly with the new reactive effect system.app/components/file/report-drawer/privacy-reports/item/index.ts (1)
10-10: LGTM! Clean refactoring to reactive effects.The migration from observers to the
useEffecthelper is implemented correctly. The effect properly tracks the realtime counter and triggers the reload action when needed. The conversion oftPleaseTryAgainto a getter is also a good optimization.Also applies to: 30-40, 75-78
app/components/project-settings/general-settings/collaborators-table/index.ts (1)
18-18: LGTM! Proper migration to reactive effects.The refactoring correctly replaces the observer pattern with
useEffect. The effect tracks the realtime counter and triggers the reload action appropriately. The initial data fetch is preserved in the constructor, ensuring data loads on component initialization.Also applies to: 43-48
app/components/partner/registration-request-pending-list/index.ts (1)
8-17: LGTM! Well-structured refactoring with proper pagination handling.The migration to
useEffectis implemented correctly. The effect tracks the realtime counter and triggers the reload action. The pagination reset logic (resetting offset when the current page becomes empty) is properly preserved. Type imports are also appropriately added.Also applies to: 38-44, 52-63
app/components/file-details/dynamic-scan/scheduled-automated/index.ts (1)
9-9: Excellent conditional effect implementation!The refactoring cleverly uses a tracked boolean flag to control when the effect should start monitoring the scan status. This ensures notifications only occur after user confirmation, preventing premature alerts. The effect properly tracks both the flag and the scan status as dependencies.
Also applies to: 29-38, 109-109, 115-119
app/components/partner/invitation-list/index.ts (1)
16-16: LGTM! Consistent refactoring pattern.The migration to
useEffectfollows the same pattern as other list components. The effect properly tracks the realtime counter, the pagination reset logic is preserved, and the initial data fetch remains in the constructor. Good consistency across the codebase.Also applies to: 37-43, 95-105
app/helpers/use-effect.ts (1)
57-106: Well-implemented effect resource with proper lifecycle management.The implementation correctly handles:
- Dependency tracking with deep equality checks
- Cleanup on both dependency changes and context destruction
- One-time execution for effects without dependencies
- Return value preservation across non-triggering renders
tests/unit/helpers/use-effect-test.js (1)
1-627: Excellent test coverage for the useEffect helper.The test suite comprehensively covers:
- Initial invocation and dependency tracking
- Cleanup lifecycle with
shouldRunCleanuppredicate- Complex dependencies including DOM nodes and objects
- Edge cases like void returns and unchanged dependencies
app/services/project.ts (1)
45-49: Consider the method chaining pattern implications.Returning
thisfromsetQueryParamsenables method chaining but is unusual for Ember actions. Ensure this pattern is used consistently and documented if it's a deliberate design choice.#!/bin/bash # Check if method chaining pattern is used elsewhere in the codebase rg -A 3 "return this;" --type tsapp/components/sbom/scan-report-drawer/report-list/index.ts (5)
5-17: LGTM: Import additions support the refactor correctly.The new imports properly support the transition from observers to the
useEffectpattern. The addition of type imports and theuseEffecthelper are necessary for the refactor.
37-42: LGTM: Well-structured reactive effect implementation.The
useEffectimplementation correctly:
- Tracks the
realtime.SbomReportCounteras a dependency- Calls the
triggerReloadLatestSbomScanReportaction when the counter changes- Follows the declarative reactive pattern consistently
This replaces the previous observer setup/teardown pattern with a cleaner, more maintainable approach.
53-55: LGTM: Performance improvement with getter conversion.Converting
tPleaseTryAgainfrom a constructor-initialized property to a getter is a good optimization. This ensures the translation is computed on-demand rather than being cached at construction time, which is more memory-efficient and handles locale changes better.
95-98: LGTM: Clean action method implementation.The
triggerReloadLatestSbomScanReportaction method properly encapsulates the task invocation and provides a clean interface for the effect to call.
100-102: LGTM: Task pattern conversion is correct.Converting
reloadLatestSbomScanReportto an ember-concurrency task is appropriate and consistent with the new reactive pattern. The task properly reloads the latest SBOM scan report.app/components/file-list/index.ts (6)
4-19: LGTM: Import additions properly support the refactor.The new imports correctly support the transition to the
useEffectpattern:
servicedecorator for dependency injection- Type imports for Store, RouterService, and IntlService
- DS type import for type definitions
useEffecthelper import- Additional type imports for pagination and query params
All imports are necessary and properly structured.
43-51: LGTM: Comprehensive reactive effect with multiple dependencies.The
observeFileCounterEffectimplementation correctly tracks multiple dependencies:
fileCounterfrom the realtime servicelimitandoffsetfrom query parametersThis ensures files are reloaded when any of these values change, providing a more comprehensive reactive solution than the previous observer pattern.
56-56: LGTM: Constructor simplification using computed getters.Using the computed
limitandoffsetgetters in the constructor is cleaner and more maintainable than passing the values directly. This ensures consistency with the reactive pattern.
100-111: LGTM: Pagination methods now properly update route state.The refactored pagination methods (
goToPageandonItemPerPageChange) now correctly update route query parameters instead of directly calling the task. This centralizes the reload logic in the reactive effect, making the data flow more predictable and easier to debug.
152-160: LGTM: Clean route query parameter update method.The
setRouteQueryParamsaction method provides a clean abstraction for updating route query parameters, maintaining consistency with the new reactive pattern.
162-180: LGTM: Simplified task implementation.The
getFilestask is now simplified and focused solely on data fetching:
- No longer handles query parameter updates
- Takes explicit limit and offset parameters
- Proper error handling with notification
- Clean separation of concerns
This simplification makes the code more maintainable and testable.
app/components/storeknox/inventory-details/app-details/va-results/index.ts (6)
9-9: LGTM: Import addition supports the refactor.The
useEffectimport is correctly added to support the transition from observers to the reactive effect pattern.
31-31: LGTM: Tracked property for reactive trigger.The
triggerReloadSkInventoryApptracked property serves as a reactive trigger to replace the observer pattern. This approach provides explicit control over when the reload effect should be triggered.
33-39: LGTM: Well-designed reactive effect with dual dependencies.The
reloadSkAppEffectimplementation correctly tracks both:
submission.status- the original observed propertytriggerReloadSkInventoryApp- a manual trigger flagThis dual dependency approach provides both automatic reactivity and manual control, which is appropriate for this use case.
88-92: LGTM: Action method with proper condition checking.The
reloadSkInventoryAppOnSubmissionStatusChangeaction properly checks both conditions before performing the reload task:
- Manual trigger flag is set
- Submission exists
This ensures the effect only runs when appropriate.
98-104: LGTM: Task conversion maintains functionality.Converting
reloadSkInventoryAppto an ember-concurrency task is correct and maintains the original functionality while fitting the new reactive pattern. The task properly reloads the SKInventoryApp and fetches the core project latest version.
110-110: LGTM: Consistent trigger flag usage.The
triggerReloadSkInventoryAppflag is appropriately set totruein multiple locations where the reload should be triggered:
- After successful app upload initiation
- After successful reload in upload process
- After successful submission resolution
This ensures the reactive effect is triggered at the right times to maintain UI consistency.
Also applies to: 121-121, 124-124, 151-151
app/components/storeknox/inventory-details/unscanned-version/table/actions/index.ts
Outdated
Show resolved
Hide resolved
Yibaebi
force-pushed
the
PD-1845-poc-implement-effect-lifecycle-method
branch
from
36e6e96 to
16d5a75
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/components/organization-invitation-list/index.ts (1)
75-80: Duplicate concern: Ensure the effect is invoked in the template.
🧹 Nitpick comments (5)
app/helpers/use-effect.ts (2)
25-25: Address the confusing void type in union.The static analysis tool correctly identifies that
voidin a union type can be confusing and suggests usingundefinedinstead.-export type EffectReturnType<T> = void | EffectCleanupObject<T>; +export type EffectReturnType<T> = undefined | EffectCleanupObject<T>;
84-84: Consider simplifying the dependency comparison logic.The current approach using
xorWithandisEqualis functional but may be overly complex. A simpler approach might be more readable and potentially more performant.Consider this alternative implementation:
-const depsChanged = !isEmpty(xorWith(depValues, prevDeps, isEqual)); +const depsChanged = depValues.length !== prevDeps.length || + depValues.some((dep, index) => !isEqual(dep, prevDeps[index]));app/components/organization-invitation-list/index.ts (2)
221-221: Apply optional chaining optimization.The static analysis tool correctly identifies that this can be simplified using optional chaining.
- errMsg = err.errors[0]?.detail || errMsg; + errMsg = err.errors?.[0]?.detail || errMsg;
169-177: Consider consolidating the reload methods.Having both
reloadInvitesEffectandreloadInvitesthat do the same thing might be confusing. Consider whether both are necessary or if one could delegate to the other for clarity.@action reloadInvitesEffect() { - this.fetchInvites.perform(); + this.reloadInvites(); } @action reloadInvites() { this.fetchInvites.perform(); }app/services/project.ts (1)
73-73: Apply optional chaining optimization.The static analysis tool correctly identifies that this can be simplified using optional chaining.
- if (err.errors && err.errors.length) { + if (err.errors?.length) {
📜 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 (48)
app/components/ak-chart/index.hbs(1 hunks)app/components/ak-chart/index.ts(2 hunks)app/components/ak-popover/index.hbs(2 hunks)app/components/ak-popover/index.ts(3 hunks)app/components/ak-tree/provider/index.hbs(1 hunks)app/components/ak-tree/provider/index.ts(2 hunks)app/components/file-details/dynamic-scan/scheduled-automated/index.hbs(1 hunks)app/components/file-details/dynamic-scan/scheduled-automated/index.ts(3 hunks)app/components/file-list/index.hbs(1 hunks)app/components/file-list/index.ts(5 hunks)app/components/file/report-drawer/privacy-reports/item/index.hbs(1 hunks)app/components/file/report-drawer/privacy-reports/item/index.ts(3 hunks)app/components/file/report-drawer/va-reports/index.hbs(1 hunks)app/components/file/report-drawer/va-reports/index.ts(3 hunks)app/components/organization-invitation-list/index.hbs(2 hunks)app/components/organization-invitation-list/index.ts(5 hunks)app/components/partner/invitation-list/index.hbs(1 hunks)app/components/partner/invitation-list/index.ts(3 hunks)app/components/partner/registration-request-pending-list/index.hbs(1 hunks)app/components/partner/registration-request-pending-list/index.ts(3 hunks)app/components/partner/registration-request-rejected-list/index.hbs(1 hunks)app/components/partner/registration-request-rejected-list/index.ts(2 hunks)app/components/project-list/index.ts(2 hunks)app/components/project-settings/general-settings/collaborators-table/index.hbs(1 hunks)app/components/project-settings/general-settings/collaborators-table/index.ts(2 hunks)app/components/project-settings/general-settings/project-team-table/index.hbs(1 hunks)app/components/project-settings/general-settings/project-team-table/index.ts(2 hunks)app/components/sbom/scan-report-drawer/report-list/index.hbs(1 hunks)app/components/sbom/scan-report-drawer/report-list/index.ts(3 hunks)app/components/security/file-search-list/index.ts(0 hunks)app/components/security/project-search-list/index.ts(0 hunks)app/components/storeknox/inventory-details/app-details/va-results/index.hbs(1 hunks)app/components/storeknox/inventory-details/app-details/va-results/index.ts(5 hunks)app/components/storeknox/inventory-details/unscanned-version/table/actions/index.hbs(1 hunks)app/components/storeknox/inventory-details/unscanned-version/table/actions/index.ts(4 hunks)app/components/upload-app/status/details/index.hbs(1 hunks)app/components/upload-app/status/details/index.ts(2 hunks)app/components/upload-app/status/index.hbs(1 hunks)app/components/upload-app/status/index.ts(4 hunks)app/helpers/use-effect.ts(1 hunks)app/services/project.ts(1 hunks)package.json(1 hunks)tests/acceptance/deprecated-routes/account-settings-redirect-test.js(1 hunks)tests/acceptance/storeknox/inventory-details/store-verson-tables-test.js(1 hunks)tests/integration/components/partner/invitation-list-test.js(0 hunks)tests/integration/components/partner/registration-request-pending-list-test.js(0 hunks)tests/integration/components/sbom/scan-report-drawer-test.js(0 hunks)tests/unit/helpers/use-effect-test.js(1 hunks)
💤 Files with no reviewable changes (5)
- app/components/security/project-search-list/index.ts
- app/components/security/file-search-list/index.ts
- tests/integration/components/partner/invitation-list-test.js
- tests/integration/components/partner/registration-request-pending-list-test.js
- tests/integration/components/sbom/scan-report-drawer-test.js
✅ Files skipped from review due to trivial changes (1)
- app/components/file/report-drawer/privacy-reports/item/index.hbs
🚧 Files skipped from review as they are similar to previous changes (39)
- app/components/upload-app/status/index.hbs
- app/components/file-details/dynamic-scan/scheduled-automated/index.hbs
- app/components/ak-tree/provider/index.hbs
- app/components/project-settings/general-settings/collaborators-table/index.hbs
- app/components/upload-app/status/details/index.hbs
- app/components/storeknox/inventory-details/app-details/va-results/index.hbs
- app/components/partner/registration-request-pending-list/index.hbs
- app/components/project-settings/general-settings/project-team-table/index.hbs
- app/components/file-list/index.hbs
- app/components/partner/invitation-list/index.hbs
- app/components/file/report-drawer/va-reports/index.hbs
- app/components/ak-chart/index.hbs
- app/components/organization-invitation-list/index.hbs
- app/components/storeknox/inventory-details/unscanned-version/table/actions/index.hbs
- app/components/ak-popover/index.hbs
- tests/acceptance/storeknox/inventory-details/store-verson-tables-test.js
- tests/acceptance/deprecated-routes/account-settings-redirect-test.js
- app/components/sbom/scan-report-drawer/report-list/index.hbs
- app/components/partner/registration-request-rejected-list/index.hbs
- app/components/project-list/index.ts
- app/components/ak-chart/index.ts
- app/components/project-settings/general-settings/project-team-table/index.ts
- app/components/upload-app/status/index.ts
- app/components/file/report-drawer/va-reports/index.ts
- app/components/file/report-drawer/privacy-reports/item/index.ts
- app/components/partner/registration-request-pending-list/index.ts
- app/components/upload-app/status/details/index.ts
- app/components/project-settings/general-settings/collaborators-table/index.ts
- app/components/partner/invitation-list/index.ts
- app/components/partner/registration-request-rejected-list/index.ts
- tests/unit/helpers/use-effect-test.js
- app/components/ak-tree/provider/index.ts
- package.json
- app/components/file-details/dynamic-scan/scheduled-automated/index.ts
- app/components/ak-popover/index.ts
- app/components/sbom/scan-report-drawer/report-list/index.ts
- app/components/storeknox/inventory-details/app-details/va-results/index.ts
- app/components/file-list/index.ts
- app/components/storeknox/inventory-details/unscanned-version/table/actions/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/components/organization-invitation-list/index.ts
[error] 221-221: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/helpers/use-effect.ts
[error] 25-25: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
app/services/project.ts
[error] 73-73: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup & Build Application
- GitHub Check: Publish to Cloudflare Pages
🔇 Additional comments (2)
app/helpers/use-effect.ts (1)
104-104: ```shell
#!/bin/bashLocate the useEffect helper definition
rg -n "function useEffect" -C 5 app/helpers/use-effect.ts
Find where previousEffectResult is declared or assigned
rg -n "previousEffectResult" -C 5 app/helpers/use-effect.ts
Show the full block around the return statement
sed -n '80,130p' app/helpers/use-effect.ts
</details> <details> <summary>app/services/project.ts (1)</summary> `44-49`: **Consider the return value utility.** The method returns `this` for chaining, but it's unclear if this pattern is used elsewhere in the codebase. Ensure this is consistent with other service methods. ```shell #!/bin/bash # Check if other service methods return 'this' for chaining rg -A 3 -B 1 "return this" app/services/
Yibaebi
force-pushed
the
PD-1845-poc-implement-effect-lifecycle-method
branch
from
16d5a75 to
2b23591
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
app/helpers/use-effect.ts (1)
94-98: The cleanup function safety has been addressed.Looking at the current implementation, the cleanup function safety issue mentioned in past review comments appears to have been properly handled. The
canRunCleanuptype guard already ensures that bothshouldRunCleanupandcleanupfunctions exist before calling them.app/components/organization-invitation-list/index.ts (1)
75-80: Verify template integration for effect activation.The
reloadInvitesEffectis properly configured to watch the realtime invitation counter. However, based on past review comments, this effect needs to be rendered in the template to activate reactive tracking.Ensure
{{this.reloadInvitesEffect}}is added to the component's template file.app/services/project.ts (1)
36-42: The filter detection logic remains potentially unsafe.The past review comment about unsafe type assertion is still valid. The current implementation could fail if
appliedFiltershas properties not present inDEFAULT_PROJECT_QUERY_PARAMS.Consider the safer approach suggested in the past review:
get isProjectReponseFiltered() { - return !Object.entries(this.appliedFilters).every( - ([key, value]) => - value === - DEFAULT_PROJECT_QUERY_PARAMS[key as keyof typeof this.appliedFilters] - ); + return !Object.entries(DEFAULT_PROJECT_QUERY_PARAMS).every( + ([key, value]) => + this.appliedFilters[key as keyof typeof DEFAULT_PROJECT_QUERY_PARAMS] === value + ); }
🧹 Nitpick comments (3)
app/helpers/use-effect.ts (1)
25-25: Address the confusing void type in union.The static analysis tool correctly identifies that
voidin union types can be confusing and should be replaced withundefinedfor better type clarity.-export type EffectReturnType<T> = void | EffectCleanupObject<T>; +export type EffectReturnType<T> = undefined | EffectCleanupObject<T>;app/components/organization-invitation-list/index.ts (1)
202-224: Simplify error handling with optional chaining.The static analysis tool correctly identifies that the error handling can be simplified using optional chaining.
- if (err.errors && err.errors.length) { - errMsg = err.errors[0]?.detail || errMsg; + if (err.errors?.length) { + errMsg = err.errors[0]?.detail || errMsg;app/services/project.ts (1)
51-81: Simplify error handling and approve task refactoring.The
fetchProjectstask has been well-refactored to use the centralizedappliedFiltersstate. However, the error handling can be simplified using optional chaining as suggested by static analysis.- if (err.errors && err.errors.length) { - errMsg = err.errors[0]?.detail || errMsg; + if (err.errors?.length) { + errMsg = err.errors[0]?.detail || errMsg;
📜 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 (48)
app/components/ak-chart/index.hbs(1 hunks)app/components/ak-chart/index.ts(2 hunks)app/components/ak-popover/index.hbs(2 hunks)app/components/ak-popover/index.ts(3 hunks)app/components/ak-tree/provider/index.hbs(1 hunks)app/components/ak-tree/provider/index.ts(2 hunks)app/components/file-details/dynamic-scan/scheduled-automated/index.hbs(1 hunks)app/components/file-details/dynamic-scan/scheduled-automated/index.ts(3 hunks)app/components/file-list/index.hbs(1 hunks)app/components/file-list/index.ts(5 hunks)app/components/file/report-drawer/privacy-reports/item/index.hbs(1 hunks)app/components/file/report-drawer/privacy-reports/item/index.ts(3 hunks)app/components/file/report-drawer/va-reports/index.hbs(1 hunks)app/components/file/report-drawer/va-reports/index.ts(3 hunks)app/components/organization-invitation-list/index.hbs(2 hunks)app/components/organization-invitation-list/index.ts(5 hunks)app/components/partner/invitation-list/index.hbs(1 hunks)app/components/partner/invitation-list/index.ts(3 hunks)app/components/partner/registration-request-pending-list/index.hbs(1 hunks)app/components/partner/registration-request-pending-list/index.ts(3 hunks)app/components/partner/registration-request-rejected-list/index.hbs(1 hunks)app/components/partner/registration-request-rejected-list/index.ts(2 hunks)app/components/project-list/index.ts(2 hunks)app/components/project-settings/general-settings/collaborators-table/index.hbs(1 hunks)app/components/project-settings/general-settings/collaborators-table/index.ts(2 hunks)app/components/project-settings/general-settings/project-team-table/index.hbs(1 hunks)app/components/project-settings/general-settings/project-team-table/index.ts(2 hunks)app/components/sbom/scan-report-drawer/report-list/index.hbs(1 hunks)app/components/sbom/scan-report-drawer/report-list/index.ts(3 hunks)app/components/security/file-search-list/index.ts(0 hunks)app/components/security/project-search-list/index.ts(0 hunks)app/components/storeknox/inventory-details/app-details/va-results/index.hbs(1 hunks)app/components/storeknox/inventory-details/app-details/va-results/index.ts(5 hunks)app/components/storeknox/inventory-details/unscanned-version/table/actions/index.hbs(1 hunks)app/components/storeknox/inventory-details/unscanned-version/table/actions/index.ts(4 hunks)app/components/upload-app/status/details/index.hbs(1 hunks)app/components/upload-app/status/details/index.ts(2 hunks)app/components/upload-app/status/index.hbs(1 hunks)app/components/upload-app/status/index.ts(4 hunks)app/helpers/use-effect.ts(1 hunks)app/services/project.ts(1 hunks)package.json(1 hunks)tests/acceptance/deprecated-routes/account-settings-redirect-test.js(1 hunks)tests/acceptance/storeknox/inventory-details/store-verson-tables-test.js(1 hunks)tests/integration/components/partner/invitation-list-test.js(0 hunks)tests/integration/components/partner/registration-request-pending-list-test.js(0 hunks)tests/integration/components/sbom/scan-report-drawer-test.js(0 hunks)tests/unit/helpers/use-effect-test.js(1 hunks)
💤 Files with no reviewable changes (5)
- app/components/security/project-search-list/index.ts
- app/components/security/file-search-list/index.ts
- tests/integration/components/partner/invitation-list-test.js
- tests/integration/components/partner/registration-request-pending-list-test.js
- tests/integration/components/sbom/scan-report-drawer-test.js
🚧 Files skipped from review as they are similar to previous changes (40)
- app/components/storeknox/inventory-details/app-details/va-results/index.hbs
- app/components/upload-app/status/details/index.hbs
- app/components/file/report-drawer/privacy-reports/item/index.hbs
- app/components/project-settings/general-settings/collaborators-table/index.hbs
- app/components/upload-app/status/index.hbs
- app/components/file-list/index.hbs
- app/components/ak-tree/provider/index.hbs
- app/components/file-details/dynamic-scan/scheduled-automated/index.hbs
- app/components/project-settings/general-settings/project-team-table/index.hbs
- app/components/partner/registration-request-pending-list/index.hbs
- tests/acceptance/storeknox/inventory-details/store-verson-tables-test.js
- app/components/partner/registration-request-rejected-list/index.hbs
- app/components/organization-invitation-list/index.hbs
- app/components/storeknox/inventory-details/unscanned-version/table/actions/index.hbs
- tests/acceptance/deprecated-routes/account-settings-redirect-test.js
- app/components/partner/invitation-list/index.hbs
- app/components/sbom/scan-report-drawer/report-list/index.hbs
- app/components/file/report-drawer/va-reports/index.hbs
- app/components/ak-chart/index.hbs
- app/components/ak-popover/index.hbs
- package.json
- app/components/ak-chart/index.ts
- app/components/project-list/index.ts
- app/components/partner/registration-request-rejected-list/index.ts
- app/components/ak-tree/provider/index.ts
- app/components/project-settings/general-settings/project-team-table/index.ts
- app/components/partner/invitation-list/index.ts
- app/components/upload-app/status/details/index.ts
- app/components/ak-popover/index.ts
- app/components/file-details/dynamic-scan/scheduled-automated/index.ts
- app/components/file/report-drawer/va-reports/index.ts
- app/components/project-settings/general-settings/collaborators-table/index.ts
- app/components/partner/registration-request-pending-list/index.ts
- tests/unit/helpers/use-effect-test.js
- app/components/file/report-drawer/privacy-reports/item/index.ts
- app/components/storeknox/inventory-details/app-details/va-results/index.ts
- app/components/upload-app/status/index.ts
- app/components/sbom/scan-report-drawer/report-list/index.ts
- app/components/file-list/index.ts
- app/components/storeknox/inventory-details/unscanned-version/table/actions/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/helpers/use-effect.ts
[error] 25-25: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
app/components/organization-invitation-list/index.ts
[error] 216-216: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/services/project.ts
[error] 73-73: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
🔇 Additional comments (7)
app/helpers/use-effect.ts (3)
37-48: LGTM - Robust type guard implementation.The
canRunCleanuptype guard is well-implemented with comprehensive runtime checks for all required properties and their types. This provides good safety for the cleanup mechanism.
74-91: Verify the dependency change detection logic.The implementation uses
xorWithwithisEqualfor dependency change detection. This approach should work correctly, but let me verify the logic:
xorWith(depValues, prevDeps, isEqual)returns elements that are different between arraysisEmpty()checks if there are any differences!isEmpty()means dependencies have changedThis logic appears sound for detecting changes in dependency arrays.
118-120: LGTM - Clean API design.The exported
useEffectfunction provides a clean API that properly wraps the resource factory and returns a reactive value. The generic typing is appropriate.app/components/organization-invitation-list/index.ts (2)
135-137: LGTM - Clean query parameter handling.The new
setQueryParamsgetter provides a clear way to determine when route query parameters should be used. This simplifies the conditional logic in pagination actions.
169-172: LGTM - Simplified reload action.The new
reloadInvitesaction is clean and focused, simply triggering the fetch task. This works well with the effect-based approach for handling realtime updates.app/services/project.ts (2)
34-34: LGTM - Centralized filter state management.Using a tracked object for
appliedFiltersis a good approach that consolidates all filter state in one place and enables reactive updates throughout the component tree.
44-49: LGTM - Fluent API design.The
setQueryParamsaction provides a clean API for updating filters and returns the service instance for potential chaining. This aligns well with the new filter management approach.
Yibaebi
force-pushed
the
PD-1845-poc-implement-effect-lifecycle-method
branch
from
2b23591 to
90e793c
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
app/helpers/use-effect.ts (1)
94-98: Cleanup function safety is already addressed.The current implementation is safe because
canRunCleanupalready verifies that bothshouldRunCleanupandcleanupexist as functions. The optional chaining is redundant but harmless.app/components/organization-invitation-list/index.ts (1)
75-80: LGTM! Effective migration from observers to reactive effects.The
reloadInvitesEffectproperly encapsulates the reactive logic that triggers invite reloading when the realtimeInvitationCounterchanges. This is a clean replacement for the manual observer pattern.app/services/project.ts (1)
36-42: Fix unsafe type assertion in filter detection logic.The current implementation uses a potentially unsafe type assertion that could fail if
appliedFilterscontains properties not present inDEFAULT_PROJECT_QUERY_PARAMS.get isProjectReponseFiltered() { - return !Object.entries(this.appliedFilters).every( - ([key, value]) => - value === - DEFAULT_PROJECT_QUERY_PARAMS[key as keyof typeof this.appliedFilters] - ); + return !Object.entries(DEFAULT_PROJECT_QUERY_PARAMS).every( + ([key, value]) => + this.appliedFilters[key as keyof typeof DEFAULT_PROJECT_QUERY_PARAMS] === value + ); }
🧹 Nitpick comments (3)
app/helpers/use-effect.ts (1)
25-25: Useundefinedinstead ofvoidin union type.The static analysis tool correctly identifies that
voidis confusing in union types. Replace withundefinedfor better type clarity.-export type EffectReturnType<T> = void | EffectCleanupObject<T>; +export type EffectReturnType<T> = undefined | EffectCleanupObject<T>;app/components/organization-invitation-list/index.ts (1)
216-216: Use optional chaining for cleaner error handling.The static analysis tool correctly suggests using optional chaining to simplify the conditional logic.
-if (err.errors && err.errors.length) { +if (err.errors?.length) {app/services/project.ts (1)
73-73: Use optional chaining for cleaner error handling.The static analysis tool correctly suggests using optional chaining to simplify the conditional logic.
-if (err.errors && err.errors.length) { +if (err.errors?.length) {
📜 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 (48)
app/components/ak-chart/index.hbs(1 hunks)app/components/ak-chart/index.ts(2 hunks)app/components/ak-popover/index.hbs(2 hunks)app/components/ak-popover/index.ts(3 hunks)app/components/ak-tree/provider/index.hbs(1 hunks)app/components/ak-tree/provider/index.ts(2 hunks)app/components/file-details/dynamic-scan/scheduled-automated/index.hbs(1 hunks)app/components/file-details/dynamic-scan/scheduled-automated/index.ts(3 hunks)app/components/file-list/index.hbs(1 hunks)app/components/file-list/index.ts(5 hunks)app/components/file/report-drawer/privacy-reports/item/index.hbs(1 hunks)app/components/file/report-drawer/privacy-reports/item/index.ts(3 hunks)app/components/file/report-drawer/va-reports/index.hbs(1 hunks)app/components/file/report-drawer/va-reports/index.ts(3 hunks)app/components/organization-invitation-list/index.hbs(2 hunks)app/components/organization-invitation-list/index.ts(5 hunks)app/components/partner/invitation-list/index.hbs(1 hunks)app/components/partner/invitation-list/index.ts(3 hunks)app/components/partner/registration-request-pending-list/index.hbs(1 hunks)app/components/partner/registration-request-pending-list/index.ts(3 hunks)app/components/partner/registration-request-rejected-list/index.hbs(1 hunks)app/components/partner/registration-request-rejected-list/index.ts(2 hunks)app/components/project-list/index.ts(2 hunks)app/components/project-settings/general-settings/collaborators-table/index.hbs(1 hunks)app/components/project-settings/general-settings/collaborators-table/index.ts(2 hunks)app/components/project-settings/general-settings/project-team-table/index.hbs(1 hunks)app/components/project-settings/general-settings/project-team-table/index.ts(2 hunks)app/components/sbom/scan-report-drawer/report-list/index.hbs(1 hunks)app/components/sbom/scan-report-drawer/report-list/index.ts(3 hunks)app/components/security/file-search-list/index.ts(0 hunks)app/components/security/project-search-list/index.ts(0 hunks)app/components/storeknox/inventory-details/app-details/va-results/index.hbs(1 hunks)app/components/storeknox/inventory-details/app-details/va-results/index.ts(5 hunks)app/components/storeknox/inventory-details/unscanned-version/table/actions/index.hbs(1 hunks)app/components/storeknox/inventory-details/unscanned-version/table/actions/index.ts(4 hunks)app/components/upload-app/status/details/index.hbs(1 hunks)app/components/upload-app/status/details/index.ts(2 hunks)app/components/upload-app/status/index.hbs(1 hunks)app/components/upload-app/status/index.ts(4 hunks)app/helpers/use-effect.ts(1 hunks)app/services/project.ts(1 hunks)package.json(1 hunks)tests/acceptance/deprecated-routes/account-settings-redirect-test.js(1 hunks)tests/acceptance/storeknox/inventory-details/store-verson-tables-test.js(1 hunks)tests/integration/components/partner/invitation-list-test.js(0 hunks)tests/integration/components/partner/registration-request-pending-list-test.js(0 hunks)tests/integration/components/sbom/scan-report-drawer-test.js(0 hunks)tests/unit/helpers/use-effect-test.js(1 hunks)
💤 Files with no reviewable changes (5)
- app/components/security/project-search-list/index.ts
- app/components/security/file-search-list/index.ts
- tests/integration/components/partner/invitation-list-test.js
- tests/integration/components/partner/registration-request-pending-list-test.js
- tests/integration/components/sbom/scan-report-drawer-test.js
✅ Files skipped from review due to trivial changes (1)
- tests/acceptance/deprecated-routes/account-settings-redirect-test.js
🚧 Files skipped from review as they are similar to previous changes (39)
- app/components/file/report-drawer/privacy-reports/item/index.hbs
- app/components/upload-app/status/details/index.hbs
- app/components/project-settings/general-settings/collaborators-table/index.hbs
- app/components/storeknox/inventory-details/app-details/va-results/index.hbs
- app/components/partner/invitation-list/index.hbs
- app/components/partner/registration-request-rejected-list/index.hbs
- app/components/file-list/index.hbs
- app/components/ak-popover/index.hbs
- app/components/storeknox/inventory-details/unscanned-version/table/actions/index.hbs
- app/components/ak-tree/provider/index.hbs
- app/components/partner/registration-request-pending-list/index.hbs
- app/components/project-settings/general-settings/project-team-table/index.hbs
- app/components/upload-app/status/index.hbs
- app/components/sbom/scan-report-drawer/report-list/index.hbs
- app/components/file-details/dynamic-scan/scheduled-automated/index.hbs
- app/components/organization-invitation-list/index.hbs
- package.json
- app/components/ak-chart/index.hbs
- app/components/file/report-drawer/va-reports/index.hbs
- app/components/project-settings/general-settings/project-team-table/index.ts
- app/components/ak-chart/index.ts
- app/components/project-list/index.ts
- app/components/project-settings/general-settings/collaborators-table/index.ts
- app/components/partner/registration-request-rejected-list/index.ts
- app/components/upload-app/status/details/index.ts
- app/components/ak-tree/provider/index.ts
- app/components/file/report-drawer/va-reports/index.ts
- app/components/file/report-drawer/privacy-reports/item/index.ts
- app/components/partner/invitation-list/index.ts
- app/components/storeknox/inventory-details/app-details/va-results/index.ts
- app/components/upload-app/status/index.ts
- tests/unit/helpers/use-effect-test.js
- app/components/partner/registration-request-pending-list/index.ts
- app/components/storeknox/inventory-details/unscanned-version/table/actions/index.ts
- tests/acceptance/storeknox/inventory-details/store-verson-tables-test.js
- app/components/file-list/index.ts
- app/components/ak-popover/index.ts
- app/components/file-details/dynamic-scan/scheduled-automated/index.ts
- app/components/sbom/scan-report-drawer/report-list/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/components/organization-invitation-list/index.ts (3)
app/helpers/use-effect.ts (1)
useEffect(118-120)app/components/ak-pagination-provider/index.ts (1)
PaginationProviderActionsArgs(15-15)types/irene/index.d.ts (1)
AdapterError(146-154)
🪛 Biome (1.9.4)
app/components/organization-invitation-list/index.ts
[error] 216-216: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/helpers/use-effect.ts
[error] 25-25: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
app/services/project.ts
[error] 73-73: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
🔇 Additional comments (10)
app/helpers/use-effect.ts (3)
37-48: LGTM! Well-structured type guard function.The
canRunCleanupfunction provides comprehensive type checking for cleanup objects with proper null/undefined handling and function type verification.
67-106: Excellent implementation of reactive effect system.The core effect logic is well-designed with proper dependency tracking using lodash's deep equality checks and appropriate cleanup handling. The separation of concerns between run-once effects and dependency-based effects is clear and correct.
118-120: LGTM! Clean API design for the useEffect hook.The exported function provides a clean interface that wraps the resource factory pattern appropriately and returns the expected reactive type.
app/components/organization-invitation-list/index.ts (4)
82-89: Constructor simplification looks good.Removing the observer setup and just calling
fetchInvites.perform()directly simplifies the initialization logic appropriately.
135-137: Good extraction of query parameter logic.The
setQueryParamsgetter provides a clean way to determine if query parameters should be managed, improving code readability.
169-172: Well-structured action for effect callback.The
reloadInvitesaction provides a clean interface for the effect to trigger the fetch task.
202-224: Task refactoring maintains good separation of concerns.The
fetchInvitestask now uses tracked properties internally while maintaining clean error handling. The conditional query parameter logic is well-structured.app/services/project.ts (3)
34-34: Good refactoring to centralize filter state.Replacing the boolean flag with a tracked
appliedFiltersobject provides better state management and more flexibility for filter handling.
44-49: Clean API design for setting query parameters.The
setQueryParamsaction provides a fluent interface by returningthis, allowing for method chaining. The implementation is clean and straightforward.
51-81: Task refactoring maintains clean separation of concerns.The
fetchProjectstask now uses internal tracked properties effectively while maintaining proper error handling and query parameter construction.
Yibaebi
force-pushed
the
PD-1845-poc-implement-effect-lifecycle-method
branch
from
90e793c to
82cea40
Compare
Yibaebi
force-pushed
the
PD-1845-poc-implement-effect-lifecycle-method
branch
from
82cea40 to
a12bd62
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/components/organization-invitation-list/index.ts (1)
83-87: Ensure the effect is invoked in the template.The
reloadInvitesDependenciesgetter properly exposes the invitation counter for reactive tracking. As mentioned in the AI summary, ensure the corresponding template wraps content in a<UseEffect>component that invokesreloadInvitesbased on these dependencies.
📜 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 (54)
.github/workflows/test.yaml(1 hunks)app/components/ak-chart/index.hbs(1 hunks)app/components/ak-chart/index.ts(1 hunks)app/components/ak-popover/index.hbs(1 hunks)app/components/ak-popover/index.ts(3 hunks)app/components/ak-tree/provider/index.hbs(1 hunks)app/components/ak-tree/provider/index.ts(1 hunks)app/components/file-details/dynamic-scan/scheduled-automated/index.hbs(1 hunks)app/components/file-details/dynamic-scan/scheduled-automated/index.ts(5 hunks)app/components/file-list/index.hbs(1 hunks)app/components/file-list/index.ts(5 hunks)app/components/file/report-drawer/privacy-reports/item/index.hbs(1 hunks)app/components/file/report-drawer/privacy-reports/item/index.ts(2 hunks)app/components/file/report-drawer/va-reports/index.hbs(1 hunks)app/components/file/report-drawer/va-reports/index.ts(2 hunks)app/components/organization-invitation-list/index.hbs(2 hunks)app/components/organization-invitation-list/index.ts(5 hunks)app/components/partner/invitation-list/index.hbs(1 hunks)app/components/partner/invitation-list/index.ts(2 hunks)app/components/partner/registration-request-pending-list/index.hbs(1 hunks)app/components/partner/registration-request-pending-list/index.ts(3 hunks)app/components/partner/registration-request-rejected-list/index.hbs(1 hunks)app/components/partner/registration-request-rejected-list/index.ts(2 hunks)app/components/project-list/index.ts(2 hunks)app/components/project-settings/general-settings/collaborators-table/index.hbs(1 hunks)app/components/project-settings/general-settings/collaborators-table/index.ts(1 hunks)app/components/project-settings/general-settings/project-team-table/index.hbs(1 hunks)app/components/project-settings/general-settings/project-team-table/index.ts(1 hunks)app/components/sbom/scan-report-drawer/report-list/index.hbs(1 hunks)app/components/sbom/scan-report-drawer/report-list/index.ts(3 hunks)app/components/security/file-search-list/index.ts(0 hunks)app/components/security/project-search-list/index.ts(0 hunks)app/components/storeknox/inventory-details/app-details/va-results/index.hbs(1 hunks)app/components/storeknox/inventory-details/app-details/va-results/index.ts(5 hunks)app/components/storeknox/inventory-details/unscanned-version/table/actions/index.hbs(1 hunks)app/components/storeknox/inventory-details/unscanned-version/table/actions/index.ts(5 hunks)app/components/storeknox/inventory/index.ts(1 hunks)app/components/upload-app/status/details/index.hbs(1 hunks)app/components/upload-app/status/details/index.ts(2 hunks)app/components/upload-app/status/index.hbs(1 hunks)app/components/upload-app/status/index.ts(3 hunks)app/components/use-effect/index.hbs(1 hunks)app/components/use-effect/index.ts(1 hunks)app/helpers/use-effect.ts(1 hunks)app/services/project.ts(1 hunks)package.json(1 hunks)tests/acceptance/deprecated-routes/account-settings-redirect-test.js(1 hunks)tests/acceptance/file-details/dynamic-scan-test.js(1 hunks)tests/acceptance/storeknox/inventory-details/store-verson-tables-test.js(1 hunks)tests/integration/components/partner/invitation-list-test.js(0 hunks)tests/integration/components/partner/registration-request-pending-list-test.js(0 hunks)tests/integration/components/sbom/scan-report-drawer-test.js(0 hunks)tests/unit/helpers/use-effect-test.js(1 hunks)types/global.d.ts(1 hunks)
💤 Files with no reviewable changes (5)
- app/components/security/file-search-list/index.ts
- app/components/security/project-search-list/index.ts
- tests/integration/components/partner/invitation-list-test.js
- tests/integration/components/partner/registration-request-pending-list-test.js
- tests/integration/components/sbom/scan-report-drawer-test.js
✅ Files skipped from review due to trivial changes (4)
- app/components/file-list/index.hbs
- app/components/file/report-drawer/va-reports/index.hbs
- app/components/use-effect/index.hbs
- app/components/use-effect/index.ts
🚧 Files skipped from review as they are similar to previous changes (38)
- app/components/upload-app/status/details/index.hbs
- tests/acceptance/deprecated-routes/account-settings-redirect-test.js
- app/components/ak-tree/provider/index.hbs
- app/components/storeknox/inventory-details/unscanned-version/table/actions/index.hbs
- app/components/file-details/dynamic-scan/scheduled-automated/index.hbs
- app/components/file/report-drawer/privacy-reports/item/index.hbs
- app/components/storeknox/inventory-details/app-details/va-results/index.hbs
- app/components/sbom/scan-report-drawer/report-list/index.hbs
- app/components/partner/registration-request-rejected-list/index.hbs
- app/components/partner/registration-request-pending-list/index.hbs
- app/components/organization-invitation-list/index.hbs
- app/components/project-settings/general-settings/project-team-table/index.hbs
- app/components/ak-chart/index.hbs
- app/components/upload-app/status/index.hbs
- tests/acceptance/storeknox/inventory-details/store-verson-tables-test.js
- app/components/partner/invitation-list/index.hbs
- app/components/ak-popover/index.hbs
- app/components/ak-tree/provider/index.ts
- app/components/ak-chart/index.ts
- package.json
- app/components/project-list/index.ts
- app/components/project-settings/general-settings/collaborators-table/index.hbs
- app/components/partner/registration-request-rejected-list/index.ts
- app/components/project-settings/general-settings/project-team-table/index.ts
- app/components/partner/registration-request-pending-list/index.ts
- app/components/file/report-drawer/va-reports/index.ts
- app/components/project-settings/general-settings/collaborators-table/index.ts
- app/components/partner/invitation-list/index.ts
- app/components/upload-app/status/details/index.ts
- app/components/storeknox/inventory-details/unscanned-version/table/actions/index.ts
- app/components/file/report-drawer/privacy-reports/item/index.ts
- app/components/file-list/index.ts
- app/components/ak-popover/index.ts
- tests/unit/helpers/use-effect-test.js
- app/components/storeknox/inventory-details/app-details/va-results/index.ts
- app/services/project.ts
- app/components/sbom/scan-report-drawer/report-list/index.ts
- app/helpers/use-effect.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/components/organization-invitation-list/index.ts (2)
app/components/ak-pagination-provider/index.ts (1)
PaginationProviderActionsArgs(15-15)types/irene/index.d.ts (1)
AdapterError(146-154)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
- GitHub Check: Cloudflare Pages
🔇 Additional comments (12)
app/components/storeknox/inventory/index.ts (1)
97-97: LGTM! Proper async/await handling added.Adding
awaitensures the toggle operation completes before showing the success notification, improving the reliability of the user feedback flow..github/workflows/test.yaml (1)
52-67: Improved test parallelization for better CI performance.The increased granularity of test execution by splitting Unit Tests and Acceptance Tests into 4 partitions each will help distribute the test workload more evenly and reduce overall CI runtime.
types/global.d.ts (1)
118-125: Enhanced type flexibility for the style modifier.The addition of
unknownto the Named argument type provides better flexibility for the style modifier to handle various value types, which aligns well with the reactive effect system implementation.tests/acceptance/file-details/dynamic-scan-test.js (1)
1077-1111: Improved test flow for better error handling coverage.The refactored test logic now properly handles both success and error scenarios after clicking the confirm button, with appropriate assertions for modal state and notification messages in each case.
app/components/upload-app/status/index.ts (2)
51-56: LGTM! Clean implementation of reactive dependencies.The
reloadSubmissionsDependenciesgetter properly exposes the realtime submission counter for reactive tracking, following the established pattern across the codebase.
96-111: Good use of const assertions for type safety.The explicit const type assertions on icon names ensure type safety and prevent accidental string mutations.
app/components/organization-invitation-list/index.ts (2)
167-170: Clean separation of concerns with the new reload action.The
reloadInvitesaction provides a clear interface for triggering data fetches, which works well with the reactive effect system.
133-135: Well-structured query parameter handling.The extraction of query parameter logic into the
setQueryParamsgetter andsetRouteQueryParamsaction improves code organization and reusability across pagination handlers.Also applies to: 174-188, 191-198
app/components/file-details/dynamic-scan/scheduled-automated/index.ts (4)
6-6: LGTM! Good practice for test integration.Adding
waitForPromiseensures async operations are properly tracked by test waiters.
48-53: Well-structured dependency getter for the effect system.The getter correctly returns functions for reactive dependency tracking, which aligns with the
useEffectpattern implementation.
104-116: Correct implementation of reactive notification logic.The refactored logic properly handles both immediate notification (when scan is already completed) and deferred notification (by enabling tracking when scan is in progress).
134-138: Async test waiter and template integration verified
- The AJAX call in
app/components/file-details/dynamic-scan/scheduled-automated/index.tsis properly wrapped withwaitForPromisefor test synchronization.- The Handlebars template
app/components/file-details/dynamic-scan/scheduled-automated/index.hbsincludes the<UseEffect>component wired tothis.notifyUserOfStatus, confirming the effect is connected correctly.All changes look good—approving.
Yibaebi
force-pushed
the
PD-1845-poc-implement-effect-lifecycle-method
branch
from
a12bd62 to
caf95eb
Compare
|
Yibaebi
force-pushed
the
PD-1845-poc-implement-effect-lifecycle-method
branch
from
caf95eb to
df22ed5
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 4
Fix all issues with AI Agents 🤖
In @app/components/ak-popover/index.hbs:
- Around line 1-6: Remove the redundant @context={{this}} prop from the
UseEffect invocation; the UseEffect component defaults to the component instance
so simply call UseEffect with @effect={{this.toggleMountContent}} and
@dependencies={{this.toggleMountContentDependencies}} (leave the
this.toggleMountContent and this.toggleMountContentDependencies references
unchanged).
In @app/components/file-details/dynamic-scan/scheduled-automated/index.ts:
- Around line 111-115: The notifyUserOfStatus action uses incorrect OR logic
causing premature notifications; update the condition in notifyUserOfStatus to
require both dynamicScanCompleted and trackDynamicScanStatus (replace the `||`
with `&&`) so that doNotifyUserOfStatus.perform() is only called when tracking
is enabled AND the dynamic scan has completed.
- Line 105: The flag trackDynamicScanStatus is set true when scheduling
notifications but never reset, so update the shutdown and task flows to clear
it: set this.trackDynamicScanStatus = false at the end of completeScanShutdown,
and ensure doNotifyUserOfStatus resets it in both success and error paths (use a
finally/final cleanup or catch handlers), and also reset it where the user
cancels the workflow so the flag cannot remain true after completion or errors.
In @app/components/organization-invitation-list/index.ts:
- Line 72: The @tracked query property is unused internally but referenced as a
fallback for extraQueryStrings.q; either remove the @tracked query declaration
or make it part of the component's public API: add a query?: string to the
component Args type (and document it in the component comment) so parent
templates can bind to it, and update the fallback usage to rely on args.query
instead of the internal field; if you choose removal, replace the fallback to
only use extraQueryStrings.q (or a safe default) and delete the @tracked query
line.
🧹 Nitpick comments (2)
app/components/partner/invitation-list/index.hbs (1)
72-91: Excellent UX improvement with conditional pagination.The pagination is now hidden when both next and previous actions are disabled (single page scenario). This is a clean implementation that improves the user experience.
Consider applying this same pattern to other paginated lists in the codebase for consistency, such as the organization-invitation-list component.
app/components/organization-invitation-list/index.ts (1)
80-80: Consider removing the constructor fetch call.The
fetchInvites.perform()call in the constructor may be redundant since the UseEffect in the template will triggerreloadInviteson initial render. While the{drop: true}option prevents duplicate fetches, removing this call could simplify the initialization flow.If you verify that the UseEffect fires reliably on initial render, consider removing line 80:
🔎 Proposed refactor
constructor( owner: unknown, args: OrganizationInvitationListSignature['Args'] ) { super(owner, args); - - this.fetchInvites.perform(); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
app/components/ak-chart/index.hbsapp/components/ak-chart/index.tsapp/components/ak-popover/index.hbsapp/components/ak-popover/index.tsapp/components/ak-tree/provider/index.hbsapp/components/ak-tree/provider/index.tsapp/components/file-details/dynamic-scan/scheduled-automated/index.hbsapp/components/file-details/dynamic-scan/scheduled-automated/index.tsapp/components/file-list/index.hbsapp/components/file-list/index.tsapp/components/file/report-drawer/privacy-reports/item/index.hbsapp/components/file/report-drawer/privacy-reports/item/index.tsapp/components/file/report-drawer/va-reports/index.hbsapp/components/file/report-drawer/va-reports/index.tsapp/components/organization-invitation-list/index.hbsapp/components/organization-invitation-list/index.tsapp/components/partner/invitation-list/index.hbsapp/components/partner/invitation-list/index.tsapp/components/partner/registration-request-pending-list/index.hbsapp/components/partner/registration-request-pending-list/index.tsapp/components/partner/registration-request-rejected-list/index.hbsapp/components/partner/registration-request-rejected-list/index.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- app/components/file-details/dynamic-scan/scheduled-automated/index.hbs
- app/components/ak-tree/provider/index.hbs
- app/components/partner/invitation-list/index.ts
- app/components/ak-popover/index.ts
- app/components/ak-chart/index.ts
- app/components/ak-chart/index.hbs
- app/components/file-list/index.ts
- app/components/file-list/index.hbs
🧰 Additional context used
🧬 Code graph analysis (1)
app/components/organization-invitation-list/index.ts (5)
app/components/ak-pagination-provider/index.ts (1)
PaginationProviderActionsArgs(15-15)app/components/file-list/index.ts (2)
limit(57-59)offset(61-63)app/components/security/file-search-list/index.ts (2)
limit(49-51)offset(53-55)app/components/security/project-search-list/index.ts (2)
limit(53-55)offset(57-59)types/irene/index.d.ts (1)
AdapterError(111-119)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
- GitHub Check: Cloudflare Pages
🔇 Additional comments (26)
app/components/partner/registration-request-rejected-list/index.ts (2)
42-47: LGTM! Clean dependency getter pattern.The dependency getter correctly exposes the reactive
RegistrationRequestCounterproperty wrapped in a function, enabling theUseEffecthelper to track changes. This aligns with the pattern used across other components in this refactor.
49-59: LGTM! Action correctly replaces the observer callback.The
reloadRegistrationRequestaction properly handles the edge case of redirecting to the first page when no items exist, and triggers a non-blocking task perform.app/components/partner/registration-request-pending-list/index.ts (1)
43-61: LGTM! Consistent implementation with the rejected-list component.The dependency getter and reload action follow the same pattern as the rejected-list component, maintaining consistency across the codebase. The page reset logic (lines 53-58) correctly handles the edge case when items are removed from the current page.
app/components/partner/registration-request-pending-list/index.hbs (2)
1-4: LGTM! Proper UseEffect wrapper integration.The
UseEffectcomponent correctly binds thereloadRegistrationRequestaction as the effect andreloadRegistrationRequestDependenciesas the dependency source, enabling reactive updates when theRegistrationRequestCounterchanges.
73-92: Clean pagination visibility logic.The use of
{{#let (and pgc.disableNext pgc.disablePrev) as |hidePagination|}}is a readable approach to conditionally hide pagination when there's only one page of results.app/components/partner/registration-request-rejected-list/index.hbs (2)
1-4: LGTM! UseEffect wrapper properly integrated.The pattern matches the pending-list component, correctly binding the effect and dependencies.
17-76: Verify: Missing loading/error/empty state handling compared to pending-list.This template only checks
{{#if this.partnerRegistrationRequestReponse}}and renders nothing otherwise. In contrast, the pending-list template explicitly handles:
- Loading state (
fetchPartnerRegistrationRequest.isRunning)- Error state (
hasErrored)- Empty state (
hasNoPartnerRegistrationRequest)If this is intentional (e.g., the rejected list should be hidden when empty rather than showing "No rejected requests"), this is fine. Otherwise, consider adding consistent state handling for better UX parity.
app/components/ak-tree/provider/index.ts (1)
83-86: LGTM! Clean refactor from observer pattern to declarative dependencies.The
initializeDependenciesdeclaration is a straightforward improvement over the previous constructor-based observer setup. The template correctly wraps the yielded block with<UseEffect>usingthis.initializeandthis.initializeDependencies, ensuring proper initialization timing (on initial render and whenevertreeDatachanges). The UseEffect helper handles cleanup viaregisterDestructor, eliminating the need for manualwillDestroylifecycle management and reducing the risk of memory leaks.app/components/file-details/dynamic-scan/scheduled-automated/index.ts (2)
47-52: > Likely an incorrect or invalid review comment.
133-137: No changes needed. The code is correct.
ember-concurrencydoes not automatically integrate with Ember's test-waiter system. UsingwaitForPromiseto wrap the AJAX call is the proper pattern for ensuring tests wait for async work within tasks. This follows the documented approach in@ember/test-waiters.Likely an incorrect or invalid review comment.
app/components/organization-invitation-list/index.hbs (2)
1-5: LGTM! UseEffect wrapper correctly implemented.The UseEffect component is properly placed at the top with the correct props to trigger
reloadInviteswhenreloadInvitesDependencieschange. This replaces the previous observer-based pattern.
53-53: All child components have been properly updated for the action rename. Bothinvite-deleteandinvite-resendcomponents have correct type signatures acceptingreloadInvites: () => voidand correctly invoke the action. No traces of the oldhandleReloadInvitesname remain in the codebase.app/components/ak-popover/index.hbs (1)
34-35: LGTM! Lifecycle hooks correctly placed.The
did-insertandwill-destroymodifiers are appropriately placed on the inner container element, aligning with the UseEffect-driven lifecycle management pattern.app/components/partner/invitation-list/index.hbs (1)
1-4: LGTM! UseEffect wrapper correctly configured.The UseEffect component properly triggers
reloadRegistrationRequestwhen dependencies change. Note that this usage doesn't include@context={{this}}(unlike the ak-popover component), which is fine if the effect doesn't require component context.app/components/organization-invitation-list/index.ts (6)
6-16: LGTM! Improved type imports.The use of type-only imports for service interfaces is a best practice that can improve build times and bundle size.
83-87: LGTM! Reactive dependencies correctly implemented.The
reloadInvitesDependenciesgetter properly returns a dependency object with a function accessor. This enables UseEffect to track changes torealtime.InvitationCounterreactively.
133-135: LGTM! Clean implementation of helper getter and action.The
setQueryParamsgetter provides a clear boolean check, and thereloadInvitesaction offers a clean public API for triggering the fetch task.Also applies to: 167-170
173-179: LGTM! Pagination handlers correctly refactored.The handlers now conditionally update route query parameters based on the component configuration and use the refactored parameterless
fetchInvitestask. The logic is clear and maintainable.Also applies to: 182-188
190-198: LGTM! Clean separation of routing logic.Extracting the route query parameter update into a dedicated action improves code organization and reusability.
200-222: LGTM! Well-structured parameterless task.The refactored
fetchInvitestask correctly derives its parameters from component state and handles errors appropriately. The{drop: true}option prevents race conditions from concurrent fetch attempts.app/components/file/report-drawer/va-reports/index.hbs (1)
1-5: LGTM! UseEffect correctly integrated.The UseEffect component is properly configured to reload reports when dependencies change, following the established pattern across the PR.
app/components/file/report-drawer/va-reports/index.ts (2)
220-222: LGTM - Clear action naming.The rename from
observeReportCountertoreloadReportsbetter reflects the action's purpose in the new UseEffect pattern.
73-77: No issues found. TheReportCounterproperty exists on theRealtimeService(line 23 ofapp/services/realtime.ts), is properly decorated with@trackedfor reactivity, and the dependency tracking pattern in the getter is correctly implemented and consistent with similar patterns elsewhere in the codebase.app/components/file/report-drawer/privacy-reports/item/index.ts (3)
34-36: Good refactor - getter improves reactivity.Converting
tPleaseTryAgainto a getter ensures translations stay current if the locale changes and is more memory efficient.
71-74: LGTM - Proper null handling.The
reloadReportsaction correctly uses optional chaining to handle the case whereprivacyReportmight be null.
29-32:PrivacyReportCounterproperty exists and is properly tracked.The property is defined in
RealtimeServiceat line 25 with the@trackeddecorator, ensuring it maintains reactivity when its value changes.
| {{! Effects }} | ||
| <UseEffect | ||
| @context={{this}} | ||
| @effect={{this.toggleMountContent}} | ||
| @dependencies={{this.toggleMountContentDependencies}} | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the UseEffect component implementation
echo "=== Searching for UseEffect component ==="
fd -t f "use-effect" --type f
echo -e "\n=== UseEffect component signature ==="
rg -n -A 20 "@context" app/components/use-effect/ 2>/dev/null || echo "Pattern not found in use-effect directory"
echo -e "\n=== All UseEffect usages in .hbs files ==="
rg -n "UseEffect" app/ --type hbs -B 1 -A 5
echo -e "\n=== Count of @context usage with UseEffect ==="
rg -n "@context.*UseEffect|UseEffect.*@context" app/ --type hbsRepository: appknox/irene
Length of output: 11958
🏁 Script executed:
cat app/helpers/use-effect.tsRepository: appknox/irene
Length of output: 3739
🏁 Script executed:
fd -name "use-effect" -type d app/components/ && \
find app/components -path "*use-effect*" -name "*.ts" -o -name "*.js" -o -name "*.hbs" 2>/dev/null | head -20Repository: appknox/irene
Length of output: 287
🏁 Script executed:
# Search for UseEffect component definition
fd "use-effect" app/components --type d
# Also check for index files in the components directory
rg -l "UseEffect" app/components --type ts | head -10
# Look for any files that export or define UseEffect component
rg "export.*UseEffect|class UseEffect" app/ --type ts --type jsRepository: appknox/irene
Length of output: 454
🏁 Script executed:
cat app/components/use-effect/index.tsRepository: appknox/irene
Length of output: 739
Remove the redundant @context={{this}} prop.
The UseEffect component defaults to the component instance (this) when @context is not provided. Explicitly passing @context={{this}} is unnecessary and inconsistent with the other 16 usages throughout the codebase that omit this prop entirely.
🤖 Prompt for AI Agents
In @app/components/ak-popover/index.hbs around lines 1-6, Remove the redundant
@context={{this}} prop from the UseEffect invocation; the UseEffect component
defaults to the component instance so simply call UseEffect with
@effect={{this.toggleMountContent}} and
@dependencies={{this.toggleMountContentDependencies}} (leave the
this.toggleMountContent and this.toggleMountContentDependencies references
unchanged).
| this, | ||
| this.notifyUserOfStatus | ||
| ); | ||
| this.trackDynamicScanStatus = true; |
There was a problem hiding this comment.
Reset trackDynamicScanStatus after the notification workflow completes.
The trackDynamicScanStatus flag is set to true when the user requests notification for an incomplete scan, but it's never reset to false after the notification is sent or the workflow is cancelled. This could cause unexpected behavior if the component lifecycle continues or the workflow is retriggered.
🔎 Suggested fix to reset the flag
Add a reset in the completeScanShutdown method:
@action async completeScanShutdown() {
this.isPerformingNotifyAction = false;
this.showNotifyUserModal = false;
+ this.trackDynamicScanStatus = false;
this.router.transitionTo(Also consider resetting on error in the doNotifyUserOfStatus task:
} catch (error) {
this.isPerformingNotifyAction = false;
+ this.trackDynamicScanStatus = false;
const errorMsg = parseError(Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @app/components/file-details/dynamic-scan/scheduled-automated/index.ts around
line 105, The flag trackDynamicScanStatus is set true when scheduling
notifications but never reset, so update the shutdown and task flows to clear
it: set this.trackDynamicScanStatus = false at the end of completeScanShutdown,
and ensure doNotifyUserOfStatus resets it in both success and error paths (use a
finally/final cleanup or catch handlers), and also reset it where the user
cancels the workflow so the flag cannot remain true after completion or errors.
| @action notifyUserOfStatus() { | ||
| if (this.dynamicScanCompleted) { | ||
| if (this.dynamicScanCompleted || this.trackDynamicScanStatus) { | ||
| this.doNotifyUserOfStatus.perform(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix incorrect OR logic that triggers notification prematurely.
The condition at line 112 uses OR logic (||), which causes the notification to be sent immediately when trackDynamicScanStatus becomes true, even if the scan hasn't completed yet.
Here's the problematic flow:
- User confirms notification while scan is incomplete (line 104)
trackDynamicScanStatusis set totrue(line 105)- The effect fires because the dependency changed
notifyUserOfStatusis called (line 111)- Line 112 evaluates:
false || true = true(scan not complete but tracking is true) - Notification is sent prematurely via
doNotifyUserOfStatus.perform()before the scan completes
The correct logic should use AND (&&) to ensure the notification is only sent when both tracking is enabled and the scan has completed.
🔎 Proposed fix
@action notifyUserOfStatus() {
- if (this.dynamicScanCompleted || this.trackDynamicScanStatus) {
+ if (this.trackDynamicScanStatus && this.dynamicScanCompleted) {
this.doNotifyUserOfStatus.perform();
}
}📝 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.
| @action notifyUserOfStatus() { | |
| if (this.dynamicScanCompleted) { | |
| if (this.dynamicScanCompleted || this.trackDynamicScanStatus) { | |
| this.doNotifyUserOfStatus.perform(); | |
| } | |
| } | |
| @action notifyUserOfStatus() { | |
| if (this.trackDynamicScanStatus && this.dynamicScanCompleted) { | |
| this.doNotifyUserOfStatus.perform(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @app/components/file-details/dynamic-scan/scheduled-automated/index.ts around
lines 111-115, The notifyUserOfStatus action uses incorrect OR logic causing
premature notifications; update the condition in notifyUserOfStatus to require
both dynamicScanCompleted and trackDynamicScanStatus (replace the `||` with
`&&`) so that doNotifyUserOfStatus.perform() is only called when tracking is
enabled AND the dynamic scan has completed.
| @service('notifications') declare notify: NotificationService; | ||
|
|
||
| @tracked inviteResponse: InvitationQueryResponse | null = null; | ||
| @tracked query = ''; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the organization-invitation-list component file
head -100 app/components/organization-invitation-list/index.ts | tail -50Repository: appknox/irene
Length of output: 1346
🏁 Script executed:
# Search for usages of organization-invitation-list component in templates
rg -n 'organization-invitation-list' --type=hbs -B2 -A2 | head -100Repository: appknox/irene
Length of output: 39
🏁 Script executed:
# Look for property bindings to query property
rg -n '\@query|query=' --type=hbs | grep -i invitation | head -50Repository: appknox/irene
Length of output: 137
🏁 Script executed:
# Check if the component file exists and show its size
wc -l app/components/organization-invitation-list/index.tsRepository: appknox/irene
Length of output: 113
🏁 Script executed:
# Let's see the area around line 201 where query is supposedly used
sed -n '195,210p' app/components/organization-invitation-list/index.tsRepository: appknox/irene
Length of output: 453
🏁 Script executed:
# Search for the component usage by looking for imports
rg -n 'organization-invitation-list' --type=ts --type=jsRepository: appknox/irene
Length of output: 1997
🏁 Script executed:
# Look for any binding pattern to the query property - more flexible search
rg -n 'query' app/components/organization-invitation-list/index.ts | head -30Repository: appknox/irene
Length of output: 550
🏁 Script executed:
# Check the component signature to see if query is exposed as a property
sed -n '1,60p' app/components/organization-invitation-list/index.tsRepository: appknox/irene
Length of output: 1941
🏁 Script executed:
# Check organization-team/invitation-list to see how it uses the component
sed -n '1,100p' app/components/organization-team/invitation-list/index.tsRepository: appknox/irene
Length of output: 2368
🏁 Script executed:
# Check the integration test to see if query is being set
cat tests/integration/components/organization-invitation-list-test.jsRepository: appknox/irene
Length of output: 7737
🏁 Script executed:
# Search for any property assignments to query within parent components
rg -n 'this.query\s*=' app/components/ --type=tsRepository: appknox/irene
Length of output: 405
🏁 Script executed:
# Check if there's a template file that might reference the query property
find app/components/organization-invitation-list -name "*.hbs" -o -name "*.gts" | xargs cat 2>/dev/null | head -50Repository: appknox/irene
Length of output: 1569
Remove the unused query property or document its intended usage.
The @tracked query property is declared at line 72 but never set internally. It's only used as a fallback in line 201 when extraQueryStrings.q is not provided. Since the property is not exposed in the component's Args interface, parent components cannot directly bind to it. If this property is not intended to be used, it should be removed; otherwise, document how parent components are expected to interact with it.
🤖 Prompt for AI Agents
In @app/components/organization-invitation-list/index.ts around line 72, The
@tracked query property is unused internally but referenced as a fallback for
extraQueryStrings.q; either remove the @tracked query declaration or make it
part of the component's public API: add a query?: string to the component Args
type (and document it in the component comment) so parent templates can bind to
it, and update the fallback usage to rely on args.query instead of the internal
field; if you choose removal, replace the fallback to only use
extraQueryStrings.q (or a safe default) and delete the @tracked query line.
Implement UseEffect Resource, update dependencies, and refactor components to use useEffect for state management and side effects.
@types/lodash,ember-resources, and updatedlodash.useEffecthelper for managing side effects instead of observers, improving performance and readability. Removed observer patterns in favour of the new effect-based approach.