Feature/dast fairplay ios17 gate - #1687
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds backend-driven DAST gating: new file attributes indicate whether DAST can start and provide a blocked message; the component short-circuits start enablement when blocked; the template wraps the start button in a tooltip showing the backend message when blocked; styles and an integration test were added. ChangesDAST Backend Blocking Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Stylelint (17.12.0)app/components/file-details/dynamic-scan/action/drawer/index.scssAllFilesIgnoredError: All input files were ignored because of the ignore pattern. Either change your input, ignore pattern or use "--allow-empty-input" to allow no inputs Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/components/file-details/dynamic-scan/action/drawer/index.hbs (1)
71-117: ⚖️ Poor tradeoffConsider eliminating the duplicated button definition.
The
AkButtoncomponent is rendered identically in both branches of the conditional (lines 84-99 and 103-116), which violates DRY principles and creates maintenance risk. If button props or content change, they must be updated in both places.Consider refactoring to render the button once and conditionally wrap it in the tooltip only when blocked.
♻️ Refactoring approach
One pattern would be to extract the button to a local
{{#let}}block or render it conditionally wrapped:This ensures the button definition stays synchronized.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/file-details/dynamic-scan/action/drawer/index.hbs` around lines 71 - 117, Duplicate AkButton markup is rendered in both branches; extract the button into a single reusable element and only conditionally wrap it with AkTooltip when this.isDastBlockedByBackend. Move the AkButton props/children (click=this.runDynamicScan, `@loading`=this.startDynamicScan.isRunning, `@disabled`=(not this.enableStartDynamicScanBtn), leftIcon with AkIcon using `@isAutomatedScan`, and the translated label) into one place (e.g., a local {{`#let`}} component or an inline partial), then render that single StartButton inside AkTooltip when isDastBlockedByBackend and directly otherwise, keeping the tooltip content sourced from this.startDynamicScanDisabledReason.app/models/file.ts (1)
55-56: 💤 Low valueConsider adding an explicit default value for clarity.
The
canStartDastattribute works correctly without a default (undefined will not equalfalsein the component check), but addingdefaultValue: truewould make the intended behavior more explicit and prevent potential confusion.📝 Suggested enhancement
- `@attr`('boolean') + `@attr`('boolean', { defaultValue: true }) declare canStartDast: boolean;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/file.ts` around lines 55 - 56, The canStartDast attribute on the model should have an explicit default to make intended behavior clear; update the `@attr`('boolean') declaration for canStartDast to include defaultValue: true so the model property is initialized true by default (locate the canStartDast attribute in the file model declaration to modify).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/components/file-details/dynamic-scan/action/drawer/index.hbs`:
- Around line 71-117: Duplicate AkButton markup is rendered in both branches;
extract the button into a single reusable element and only conditionally wrap it
with AkTooltip when this.isDastBlockedByBackend. Move the AkButton
props/children (click=this.runDynamicScan,
`@loading`=this.startDynamicScan.isRunning, `@disabled`=(not
this.enableStartDynamicScanBtn), leftIcon with AkIcon using `@isAutomatedScan`,
and the translated label) into one place (e.g., a local {{`#let`}} component or an
inline partial), then render that single StartButton inside AkTooltip when
isDastBlockedByBackend and directly otherwise, keeping the tooltip content
sourced from this.startDynamicScanDisabledReason.
In `@app/models/file.ts`:
- Around line 55-56: The canStartDast attribute on the model should have an
explicit default to make intended behavior clear; update the `@attr`('boolean')
declaration for canStartDast to include defaultValue: true so the model property
is initialized true by default (locate the canStartDast attribute in the file
model declaration to modify).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b00a5a03-4df7-46cf-a889-d60664f6199f
📒 Files selected for processing (6)
app/components/file-details/dynamic-scan/action/drawer/index.hbsapp/components/file-details/dynamic-scan/action/drawer/index.scssapp/components/file-details/dynamic-scan/action/drawer/index.tsapp/models/file.tsmirage/factories/file.tstests/integration/components/file-details/dynamic-scan/manual-test.js
Irene
 |
||||||||||||||||||||||||||||||||||||||||
| Project |
Irene
|
| Branch Review |
feature/dast-fairplay-ios17-gate
|
| Run status |
|
| Run duration | 02m 46s |
| Commit |
|
| Committer | Samartha J V |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
2
|
 |
0
|
 |
1
|
 |
7
|
 |
22
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
| Test | Artifacts | |
|---|---|---|
| Ignore Vulnerability > It ignores vulnerability from Risk --> Passed (CRITERIA: "current file") (RESET: for "Current File Only") |
Test Replay
Screenshots
|
|
| Test | Artifacts | |
|---|---|---|
| Dynamic Scan > it tests dynamic scan for an apk file: 132571 |
Test Replay
Screenshots
|
|
Expose canStartDast, dastBlockedReason, and dastBlockedMessage on the file model so the dynamic scan drawer can rely on the backend eligibility verdict. Disable the Start button when DAST is blocked, show the backend-provided reason in the disabled-state tooltip, and keep the button markup centralized under a disabled AkTooltip wrapper. Add Mirage defaults and an integration test for the blocked start state.
8148a81 to
80698fb
Compare
Deploying irenestaging with
|
| Latest commit: |
80698fb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://36d66e6c.irenestaging.pages.dev |
| Branch Preview URL: | https://feature-dast-fairplay-ios17.irenestaging.pages.dev |
|
2 participants
Frontend gate for the FairPlay iOS 17+ DAST block
canStartDast,dastBlockedReason, anddastBlockedMessageattributes to the file model.canStartDast=false, disable the start / schedule-automation button in the dynamic scan drawer and wrap it in a tooltip showingdastBlockedMessage.