Select/unselect all functionality to captured apis table #1523
Conversation
WalkthroughThe pull request modifies the file details API scan component to introduce functionality for selecting and unselecting all captured APIs. Changes include updates to the Handlebars template, TypeScript component, translations, and tests. The implementation enhances the loading state rendering and adds UI elements for bulk selection, alongside backend logic to manage the selection state and server interactions. Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Yibaebi
force-pushed
the
update-api-scan-page
branch
from
0bf7183 to
e4612d7
Compare
Deploying irenestaging with
|
| Latest commit: |
d385b08
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a2ec29ef.irenestaging.pages.dev |
| Branch Preview URL: | https://update-api-scan-page.irenestaging.pages.dev |
Irene
|
||||||||||||||||||||||||||||||||||
| Project |
Irene
|
| Branch Review |
update-api-scan-page
|
| Run status |
|
| Run duration | 04m 59s |
| Commit |
|
| Committer | Yibaebi Elliot |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
31
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/tests/dynamic-scan.spec.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| Dynamic Scan > it tests dynamic scan for an ipa file: 58061 |
Test Replay
Screenshots
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/components/file-details/api-scan/captured-apis/index.hbs(1 hunks)app/components/file-details/api-scan/captured-apis/index.ts(6 hunks)tests/acceptance/file-details/api-scan-test.js(1 hunks)translations/en.json(1 hunks)translations/ja.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Push Docker image to GitHub Packages
- GitHub Check: Cloudflare Pages
🔇 Additional comments (8)
translations/en.json (1)
1406-1406: LGTM! Clear and consistent translation entry.The translation key and value follow the established conventions and clearly communicate the bulk selection action.
app/components/file-details/api-scan/captured-apis/index.hbs (1)
35-48: LGTM! Well-structured UI implementation for bulk selection.The implementation follows good practices:
- Uses consistent component library (Ak* components)
- Has proper layout structure with AkStack
- Includes data-test attributes for testing
- Places the select all control logically before the list
app/components/file-details/api-scan/captured-apis/index.ts (5)
45-45: LGTM! Well-typed tracked property.The tracked property is properly declared for managing the bulk selection state.
73-79: LGTM! Clean endpoint construction.The endpoint construction method follows good practices:
- Uses array join for clean URL construction
- Leverages environment configuration
- Has clear naming
110-113: LGTM! Well-structured event handler.The event handler follows good practices:
- Uses proper typing for event and checked parameters
- Delegates to a task for async operations
170-185: LGTM! Robust task implementation for bulk selection.The task implementation follows best practices:
- Uses proper error handling with parseError
- Updates UI state after API calls
- Refreshes the list to reflect changes
187-203: LGTM! Well-implemented status check task.The task implementation:
- Uses proper typing for the API response
- Handles errors appropriately
- Updates the UI state correctly
tests/acceptance/file-details/api-scan-test.js (1)
264-350: LGTM! Comprehensive test coverage for bulk selection.The test implementation follows best practices:
- Uses test.each for testing both selection states
- Has proper server mocks
- Includes thorough assertions
- Verifies UI state changes
Yibaebi
force-pushed
the
update-api-scan-page
branch
from
e4612d7 to
b23ca58
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/components/file-details/api-scan/captured-apis/index.ts (2)
111-114: Consider refining the event parameter type.While the implementation is correct, consider using a more specific type for the event parameter:
- handleSelectAllCapturedApis(_: Event, checked: boolean) { + handleSelectAllCapturedApis(_: MouseEvent | KeyboardEvent, checked: boolean) {
192-208: Improve error handling and add specific error messages.The error handling could be improved with better type annotations and more specific error messages.
getAllAPIsSelectedStatus = task(async () => { try { const allAPIsSelected = await this.ajax.request<{ is_active: boolean }>( this.modAllSelectedAPIsEndpoint, { namespace: ENV.namespace_v2, } ); this.allAPIsSelected = allAPIsSelected.is_active; } catch (err) { - const error = err as AdapterError; + const error = err as AjaxError; - const errMsg = this.intl.t('tPleaseTryAgain'); + const errMsg = this.intl.t('errorFetchingSelectionStatus'); this.notify.error(parseError(error, errMsg)); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/components/file-details/api-scan/captured-apis/index.hbs(2 hunks)app/components/file-details/api-scan/captured-apis/index.scss(1 hunks)app/components/file-details/api-scan/captured-apis/index.ts(6 hunks)tests/acceptance/file-details/api-scan-test.js(1 hunks)translations/en.json(1 hunks)translations/ja.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/components/file-details/api-scan/captured-apis/index.scss
🚧 Files skipped from review as they are similar to previous changes (3)
- translations/en.json
- translations/ja.json
- app/components/file-details/api-scan/captured-apis/index.hbs
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Run Integration Tests - Part 8
- GitHub Check: Run Integration Tests - Part 7
- GitHub Check: Run Integration Tests - Part 6
- GitHub Check: Run Integration Tests - Part 5
- GitHub Check: Run Integration Tests - Part 4
- GitHub Check: Run Integration Tests - Part 3
- GitHub Check: Run Integration Tests - Part 2
- GitHub Check: Run Integration Tests - Part 1
- GitHub Check: Run Acceptance Tests
- GitHub Check: Run Linting
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
tests/acceptance/file-details/api-scan-test.js (1)
264-350: LGTM! Comprehensive test coverage for select/unselect functionality.The test implementation is thorough and well-structured:
- Uses test.each to verify both selection states
- Properly mocks server responses
- Verifies UI state changes
- Checks checkbox states for all API endpoints
app/components/file-details/api-scan/captured-apis/index.ts (3)
13-13: LGTM! Well-typed tracked properties.The new tracked properties and import are properly defined with appropriate types for managing the selection and loading states.
Also applies to: 45-46
57-57: LGTM! Proper initialization sequence.The constructor correctly initializes the selection state by performing the
getAllAPIsSelectedStatustask.
74-80: LGTM! Safe URL construction.The endpoint URL is constructed safely using array join with proper typing.
Yibaebi
force-pushed
the
update-api-scan-page
branch
from
b23ca58 to
c9133a5
Compare
Yibaebi
force-pushed
the
update-api-scan-page
branch
from
c9133a5 to
d385b08
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/components/file-details/api-scan/captured-apis/index.ts (1)
173-190:⚠️ Potential issueFix loading state management.
The loading state management is reversed:
showAPIListLoadingState = falsebefore operation startsshowAPIListLoadingState = truein finally blockselectAllCapturedApis = task(async (is_active: boolean) => { - this.showAPIListLoadingState = false; + this.showAPIListLoadingState = true; try { await this.ajax.put(this.modAllSelectedAPIsEndpoint, { data: { is_active: is_active }, namespace: ENV.namespace_v2, }); await this.fetchCapturedApis.perform(this.limit, this.offset); await this.getAllAPIsSelectedStatus.perform(); this.setSelectedApiCount.perform(); } catch (err) { this.notify.error(parseError(err, this.intl.t('tPleaseTryAgain'))); } finally { - this.showAPIListLoadingState = true; + this.showAPIListLoadingState = false; } });
🧹 Nitpick comments (3)
app/components/file-details/api-scan/captured-apis/index.ts (3)
74-80: Consider a more descriptive property name.The property name could be more specific about its purpose, e.g.,
toggleAllCapturedApisEndpointorbulkSelectionEndpoint.
111-114: Improve type safety of the event parameter.Consider using a more specific event type for better type safety:
- handleSelectAllCapturedApis(_: Event, checked: boolean) { + handleSelectAllCapturedApis(_: MouseEvent | KeyboardEvent, checked: boolean) {
Line range hint
157-169: Simplify error handling using parseError utility.For consistency with other error handlers in the component, consider using the parseError utility:
} catch (err) { - const error = err as AdapterError; - let errMsg = this.intl.t('tPleaseTryAgain'); - - if (error.errors && error.errors.length) { - errMsg = error.errors[0]?.detail || errMsg; - } else if (error.message) { - errMsg = error.message; - } - - this.notify.error(errMsg); + this.notify.error(parseError(err, this.intl.t('tPleaseTryAgain'))); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/components/file-details/api-scan/captured-apis/index.hbs(2 hunks)app/components/file-details/api-scan/captured-apis/index.scss(1 hunks)app/components/file-details/api-scan/captured-apis/index.ts(7 hunks)app/styles/_component-variables.scss(1 hunks)tests/acceptance/file-details/api-scan-test.js(1 hunks)translations/en.json(1 hunks)translations/ja.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/styles/_component-variables.scss
- app/components/file-details/api-scan/captured-apis/index.scss
- app/components/file-details/api-scan/captured-apis/index.hbs
- translations/ja.json
- translations/en.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Push Docker image to GitHub Packages
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
tests/acceptance/file-details/api-scan-test.js (1)
264-348: LGTM! Well-structured test case.The test case thoroughly verifies the select/unselect all functionality by:
- Testing both selection states
- Setting up appropriate server responses
- Verifying UI state changes
app/components/file-details/api-scan/captured-apis/index.ts (2)
45-46: LGTM! Well-structured initialization.The tracked properties are appropriately named and the constructor correctly initializes the component state by fetching the selection status.
Also applies to: 57-57
192-205: LGTM! Well-implemented status check.The task has proper error handling using parseError and good type safety with the response type annotation.
|
|
||
| await this.fetchCapturedApis.perform(this.limit, this.offset); | ||
| await this.getAllAPIsSelectedStatus.perform(); | ||
| this.setSelectedApiCount.perform(); |
There was a problem hiding this comment.
add a new line above
| @@ -901,7 +901,7 @@ body { | |||
| --file-details-api-scan-captured-apis-border-radius: var(--border-radius); | |||
| --file-details-api-scan-captured-apis-card-box-shadow: var(--box-shadow-3); | |||
| --file-details-api-scan-captured-apis-card-background: var(--background-main); | |||
| --file-details-api-scan-captured-apis-title-background: var( | |||
| --file-details-api-scan-select-all-captured-api-background: var( | |||
There was a problem hiding this comment.
name should be --file-details-api-scan-captured-apis-select-all-background
| }); | ||
|
|
||
| await this.fetchCapturedApis.perform(this.limit, this.offset); | ||
| await this.getAllAPIsSelectedStatus.perform(); |
There was a problem hiding this comment.
why do we require this call ?
No description provided.