ADD: DAST Screenshots in vulnerability details page #1618
Conversation
WalkthroughAdds a DetailedAnalysis data model, adapter, Mirage fixtures, screenshot UI (button + drawer + image loading) in vulnerability findings, fixes repeated CSS classname typos, introduces Iconoir screenshot icon and build/type support, adds CSS variables and translations, and updates tests to cover detailed-analysis and screenshots. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
mirage/models/detailed-analysis.ts (1)
1-3: Minimal Mirage model is fine; optional typing for DX.You can keep as-is. Optionally, add TS typing via an interface for attributes if you later extend it.
app/components/file-details/vulnerability-analysis-details/index.hbs (1)
274-275: Gate rendering to avoid unnecessary fetches/404s.Render Screenshots only for DAST (or when screenshots exist) to prevent extra API calls for SAST/API analyses.
Consider wrapping with a guard like “if isDAST” or “if hasScreenshots”. Verify the component itself doesn’t fetch when not applicable.
app/models/detailed-analysis.ts (1)
3-6: Provide a default for screenshots to avoid undefined.Set a default empty array to simplify consumers and reduce defensive checks.
Apply this diff:
-export default class DetailedAnalysisModel extends Model { - @attr() - declare screenshots: string[]; -} +export default class DetailedAnalysisModel extends Model { + @attr({ defaultValue: () => [] as string[] }) + declare screenshots: string[]; +}mirage/factories/detailed-analysis.ts (1)
4-12: Harden factory definitions for clarity and safety.
- Define id as a function to match Mirage patterns.
- Prefer Array.from for generating arrays.
-export default Factory.extend({ - id: faker.string.uuid, +export default Factory.extend({ + id() { + return faker.string.uuid(); + }, screenshots: () => { const count = faker.number.int({ min: 0, max: 5 }); - return Array(count) - .fill(null) - .map(() => faker.image.url({ width: 800, height: 600 })); + return Array.from({ length: count }, () => + faker.image.url({ width: 800, height: 600 }) + ); }, });app/components/file-details/screenshots/index.hbs (3)
31-33: i18n: include ellipsis in translation, not the template.Appending "..." in template breaks localization. Move it into the 'loading' translation key (or a new 'loadingEllipsis').
- <AkTypography>{{t 'loading'}}...</AkTypography> + <AkTypography>{{t 'loadingEllipsis'}}</AkTypography>
104-114: Improve alt text for accessibility.Use a descriptive alt like “Screenshot X of Y” to aid screen readers.
- alt={{t 'screenshots'}} + alt={{t 'screenshotNumberOfTotal' number=(add this.currentIndex 1) total=this.screenshots.length}}Note: add 'screenshotNumberOfTotal' to translations (e.g., "Screenshot {{number}} of {{total}}").
47-51: Avoid inline styles; move icon font-size to CSS.Inline styles with !important reduce maintainability. Prefer a local-class and SCSS rule.
- <AkIcon - @iconName='arrow-left' - {{style fontSize='1.28rem !important'}} - /> + <AkIcon @iconName='arrow-left' local-class='screenshot-nav-icon' /> @@ - <AkIcon - @iconName='arrow-right' - {{style fontSize='1.28rem !important'}} - /> + <AkIcon @iconName='arrow-right' local-class='screenshot-nav-icon' />And in SCSS:
.screenshot-nav-icon { font-size: 1.28rem; }Also applies to: 77-81
app/components/file-details/screenshots/index.scss (3)
23-34: Make sticky offset configurable.Hard-coded top: calc(132px + 1.5em) is brittle. Expose a CSS var so layout changes don’t require code edits.
- top: calc(132px + 1.5em); + top: var(--file-details-sticky-title-offset, calc(132px + 1.5em));Also consider moving z-index to a var if shared elsewhere.
61-69: Avoid unnecessary !important on border-radius.The specificity seems controllable; prefer raising selector specificity over !important.
77-81: Consider responsive max-height.Use clamp to better adapt to viewport heights.
- max-height: 500px; + max-height: clamp(240px, 50vh, 600px);app/components/file-details/screenshots/index.ts (2)
18-19: Remove unused intl service injection.It’s not referenced in the component logic.
- @service declare intl: IntlService;
41-43: Type the analysis getter for clarity.Make the return type explicit.
- get analysis() { + get analysis(): AnalysisModel | null { return this.args.analysis || null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/adapters/detailed-analysis.ts(1 hunks)app/components/file-details/screenshots/index.hbs(1 hunks)app/components/file-details/screenshots/index.scss(1 hunks)app/components/file-details/screenshots/index.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/index.hbs(1 hunks)app/models/detailed-analysis.ts(1 hunks)app/styles/_component-variables.scss(1 hunks)mirage/factories/detailed-analysis.ts(1 hunks)mirage/models/detailed-analysis.ts(1 hunks)tests/integration/components/file-details/screenshots/index-test.js(1 hunks)translations/en.json(1 hunks)translations/ja.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/integration/components/file-details/screenshots/index-test.js (1)
app/components/file-details/screenshots/index.ts (1)
analysis(41-43)
app/components/file-details/screenshots/index.ts (2)
app/models/analysis.ts (1)
AnalysisModel(45-291)tests/integration/components/file-details/screenshots/index-test.js (1)
screenshots(49-52)
⏰ 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). (6)
- GitHub Check: Run E2E Tests (4)
- GitHub Check: Run E2E Tests (5)
- GitHub Check: Run E2E Tests (3)
- GitHub Check: Run E2E Tests (1)
- GitHub Check: Run E2E Tests (2)
- GitHub Check: Setup & Build Application
🔇 Additional comments (4)
translations/en.json (1)
1783-1783: LGTM for new label.app/styles/_component-variables.scss (1)
1187-1195: Screenshots theme variables look complete and consistent.All variables used by the component SCSS are defined here. LGTM.
tests/integration/components/file-details/screenshots/index-test.js (2)
33-46: Good: validates empty state. Add an assertion for container absence only (already present).Looks solid.
48-93: Strengthen screenshot navigation tests
- Assert
[data-test-screenshots-prev-button]is disabled on first screenshot and[data-test-screenshots-next-button]disabled on last.- Ensure your Mirage
detailed-analysisroute returns a JSON:API–compliant payload (or add a custom serializer) soresponse.screenshotsloads correctly.Proposed additions:
@@ assert.dom('[data-test-screenshots-prev-button]').exists(); + assert.dom('[data-test-screenshots-prev-button]').isDisabled(); assert.dom('[data-test-screenshots-next-button]').exists(); @@ await click('[data-test-screenshots-next-button]'); + assert.dom('[data-test-screenshots-next-button]').isDisabled();Verify serializer setup with:
rg -n "app/serializers/detailed-analysis" -g "app/serializers"
avzz-19
force-pushed
the
dast-screenshots
branch
from
18b6790 to
bded08d
Compare
Deploying irenestaging with
|
| Latest commit: |
15d8cbd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e2769874.irenestaging.pages.dev |
| Branch Preview URL: | https://dast-screenshots.irenestaging.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
app/components/file-details/screenshots/index.ts (4)
30-38: Handle promise rejections in constructor and swallow image-load errors.Add .catch to loadScreenshots.perform(...) and guard loadImage(...) to avoid unhandled rejections and sticky loading state.
- if (this.analysis?.id) { - this.loadScreenshots.perform(this.analysis.id).then((screenshots) => { - this.screenshots = screenshots; - - if (this.currentScreenshot) { - this.loadImage(this.currentScreenshot); - } - }); - } + if (this.analysis?.id) { + void this.loadScreenshots + .perform(this.analysis.id) + .then((screenshots) => { + this.screenshots = screenshots; + if (this.currentScreenshot) { + void this.loadImage(this.currentScreenshot).catch(() => { + // ignore image failures; UI already resets loading state + }); + } + }) + .catch(() => { + this.screenshots = []; + this.isLoadingImage = false; + }); + }
105-112: Avoid unhandled rejection on index change.Catch loadImage(...) to prevent console noise on image failures.
- if (this.currentScreenshot) { - this.loadImage(this.currentScreenshot); - } + if (this.currentScreenshot) { + void this.loadImage(this.currentScreenshot).catch(() => {}); + }
114-118: Make task resilient to fetch failures.Wrap findRecord in try/catch so the constructor chain doesn’t reject.
- loadScreenshots = task(async (id: string) => { - const response = await this.store.findRecord('detailed-analysis', id); - - return response.screenshots || []; - }); + loadScreenshots = task(async (id: string) => { + try { + const response = await this.store.findRecord('detailed-analysis', id); + return response.screenshots || []; + } catch { + return []; + } + });
97-103: Release resources on teardown.Add willDestroy to clean up any in‑flight image handlers.
// insert before the class closing brace willDestroy(): void { super.willDestroy(); this.cleanupLoadingImage(); }
🧹 Nitpick comments (7)
app/components/file-details/screenshots/index.ts (1)
41-43: Tighten typing of analysis getter.Signature requires analysis; returning null widens type unnecessarily. Either make the arg optional everywhere or return the exact type.
-get analysis() { - return this.args.analysis || null; -} +get analysis(): AnalysisModel { + return this.args.analysis; +}app/components/file-details/screenshots/index.hbs (2)
94-115: Remove redundant conditional.Outer block already guards on screenshots length; the inner {{#if this.screenshots.length}} is unnecessary.
- {{#if this.screenshots.length}} {{#if this.isLoadingImage}} <div local-class='loading-container'> <AkStack @alignItems='center' @spacing='2'> <AkLoader @size={{16}} /> <AkTypography>{{t 'loading'}}...</AkTypography> </AkStack> </div> {{/if}} {{#if this.currentScreenshot}} <img src={{this.currentScreenshot}} - alt={{t 'screenshots'}} + alt={{t 'screenshots'}} local-class='screenshot-image {{if this.isLoadingImage "ak-display-none" }}' data-test-screenshot-image /> {{/if}} - {{/if}}
104-113: Improve image accessibility and loading behavior.Provide better alt text and modern loading hints.
- <img - src={{this.currentScreenshot}} - alt={{t 'screenshots'}} + <img + src={{this.currentScreenshot}} + alt={{t "screenshotOf" index=(add this.currentIndex 1) total=this.screenshots.length}} + decoding="async" + loading="eager" + draggable="false"Follow-up: add i18n key screenshotOf like “Screenshot {{index}} of {{total}}”.
tests/integration/components/file-details/screenshots/index-test.js (4)
1-7: Wait for async rendering to settle.Import and use settled() to avoid race conditions with async data/tasks.
-import { render, click } from '@ember/test-helpers'; +import { render, click, settled } from '@ember/test-helpers';
39-42: Settle after render (no screenshots).Prevents flakiness when tasks resolve on the next tick.
await render(hbs` <FileDetails::Screenshots @analysis={{this.analysis}} /> `); + await settled();
59-62: Settle after render (with screenshots).Same rationale as above; ensures DOM reflects loaded state.
await render(hbs` <FileDetails::Screenshots @analysis={{this.analysis}} /> `); + await settled();
71-79: Assert navigation button disabled states.Verify prev is disabled on first, next disabled on last.
assert.dom('[data-test-screenshots-prev-button]').exists(); assert.dom('[data-test-screenshots-next-button]').exists(); + assert.dom('[data-test-screenshots-prev-button]').isDisabled(); + assert.dom('[data-test-screenshots-next-button]').isNotDisabled(); // Navigate to next screenshot await click('[data-test-screenshots-next-button]'); + await settled(); + assert.dom('[data-test-screenshots-prev-button]').isNotDisabled(); // Navigate back to first screenshot await click('[data-test-screenshots-prev-button]'); + await settled(); + assert.dom('[data-test-screenshots-prev-button]').isDisabled();Also applies to: 80-86, 88-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/adapters/detailed-analysis.ts(1 hunks)app/components/file-details/screenshots/index.hbs(1 hunks)app/components/file-details/screenshots/index.scss(1 hunks)app/components/file-details/screenshots/index.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/index.hbs(1 hunks)app/models/detailed-analysis.ts(1 hunks)app/styles/_component-variables.scss(1 hunks)mirage/factories/detailed-analysis.ts(1 hunks)mirage/models/detailed-analysis.ts(1 hunks)tests/integration/components/file-details/screenshots/index-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 (9)
- app/components/file-details/screenshots/index.scss
- mirage/models/detailed-analysis.ts
- app/adapters/detailed-analysis.ts
- mirage/factories/detailed-analysis.ts
- app/models/detailed-analysis.ts
- translations/ja.json
- app/components/file-details/vulnerability-analysis-details/index.hbs
- app/styles/_component-variables.scss
- translations/en.json
🧰 Additional context used
🧬 Code graph analysis (2)
tests/integration/components/file-details/screenshots/index-test.js (1)
app/components/file-details/screenshots/index.ts (1)
analysis(41-43)
app/components/file-details/screenshots/index.ts (2)
app/models/analysis.ts (1)
AnalysisModel(45-291)tests/integration/components/file-details/screenshots/index-test.js (1)
screenshots(49-52)
⏰ 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). (2)
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/components/file-details/screenshots/index.ts (1)
30-34: Guard args access and handle task rejection to avoid console noise.Directly reading this.analysis.id can throw if args are missing; task rejections go uncaught.
constructor(owner: unknown, args: FileDetailsScreenshotsSignature['Args']) { super(owner, args); - this.loadScreenshots.perform(this.analysis.id); + if (this.analysis?.id) { + // catch to prevent unhandled rejections in tests/runtime + void this.loadScreenshots + .perform(this.analysis.id) + .catch(() => { + this.screenshots = []; + this.isLoadingImage = false; + }); + } else { + this.isLoadingImage = false; + } }
🧹 Nitpick comments (5)
app/components/file-details/screenshots/index.scss (3)
23-34: Avoid hard‑coded sticky offset; use a themable CSS variable.Top offset uses calc(132px + 1.5em) which may drift with header changes.
- top: calc(132px + 1.5em); + top: var(--file-details-screenshots-sticky-top, calc(132px + 1.5em));
48-60: Improve disabled button UX (cursor/opacity) without changing visuals.Current rule equalizes hover/disabled background only. Add disabled affordances.
.ak-pagination-prev-button, .ak-pagination-next-button { height: 30px; background-color: var(--file-details-screenshots-bg-color-white); min-width: unset; padding-left: 0.143em; padding-right: 0.857em; &:hover, &:disabled { background-color: var(--file-details-screenshots-bg-color-white) !important; } + &:disabled { + cursor: not-allowed; + opacity: 0.6; + } }
78-82: Make image height responsive to viewport.Fixed 500px can overflow small screens.
.screenshot-image { max-width: 100%; - max-height: 500px; + max-height: min(60vh, 500px); object-fit: contain; }app/components/file-details/screenshots/index.ts (2)
133-171: Optional: provide a retry action on error.Right now, imageLoadError is terminal until navigation. Consider a small retry that reuses loadCurrentImage.
@action retryCurrentImage() { this.imageLoadError = false; this.isLoadingImage = true; this.loadCurrentImage(); }Hook it to a “Retry” button in the template near the error state.
11-15: Type the component signature args as readonly to prevent accidental mutation.Minor safety/intent improvement.
export interface FileDetailsScreenshotsSignature { Args: { - analysis: AnalysisModel; + readonly analysis: AnalysisModel; }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/components/file-details/screenshots/index.scss(1 hunks)app/components/file-details/screenshots/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/components/file-details/screenshots/index.ts (1)
app/models/analysis.ts (1)
AnalysisModel(45-291)
🔇 Additional comments (3)
app/components/file-details/screenshots/index.ts (2)
196-201: Cleanup is good.willDestroy clears handlers and caches. Nice.
21-26: No changes needed.The
limitproperty is actively used in the template (app/components/file-details/screenshots/index.hbs:17), so the original guidance applies: "If used in hbs, ignore."app/components/file-details/screenshots/index.scss (1)
84-86: Verification complete: No utility duplication exists.The search confirmed that
.ak-display-noneis defined once atapp/components/file-details/screenshots/index.scss:84and used once in the same component's template atapp/components/file-details/screenshots/index.hbs:110. No similar utility classes (.is-hidden,.hidden,.display-none,.d-none) exist elsewhere in the codebase, so cascade quirks from redefinition are not a concern. The implementation is valid.
avzz-19
force-pushed
the
dast-screenshots
branch
2 times, most recently
from
eb0a15e to
eed71ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
app/components/file-details/screenshots/index.scss (1)
5-11: Previous typo concern already resolved.The past review comment flagged a typo
analysis-overridded-passed, but the current code correctly usesanalysis-overridden-passed. No further action needed.app/components/file-details/screenshots/index.ts (3)
30-34: Handle promise rejection in constructor.The call to
this.loadScreenshots.perform(this.analysis.id)on line 33 does not handle promise rejection. If the task fails (e.g., network error), this results in an unhandled promise rejection and potential console errors.Apply a
.catch()handler to handle or log errors gracefully:constructor(owner: unknown, args: FileDetailsScreenshotsSignature['Args']) { super(owner, args); - this.loadScreenshots.perform(this.analysis.id); + this.loadScreenshots + .perform(this.analysis.id) + .catch(() => { + // Task failure is handled within the task itself + }); }
77-91: Prevent perpetual spinner when no screenshot is available.The early return on line 80 when
!this.currentScreenshotdoes not resetisLoadingImagetofalse. This causes a perpetual loading spinner when the screenshots array is empty or the current index is invalid.Apply this fix:
@action updateLoadingStateForCurrentImage() { if (!this.currentScreenshot) { + this.isLoadingImage = false; + this.imageLoadError = false; return; } // Only update loading state based on current image if (this.loadedImages.has(this.currentScreenshot)) { this.isLoadingImage = false; this.imageLoadError = false; } else { this.isLoadingImage = true; this.imageLoadError = false; } }
183-194: Add error handling and reset state when loading screenshots.The task has several issues:
- Line 189:
store.findRecordis not wrapped in try/catch, leading to unhandled rejections on network errors.currentIndexis not reset when new data loads, potentially pointing beyond the array bounds.- No cleanup of loading state on error.
Apply this fix:
loadScreenshots = task(async (id?: string) => { // If no id is provided, do nothing if (!id) { + this.screenshots = []; + this.currentIndex = 0; + this.isLoadingImage = false; + this.imageLoadError = false; return; } - const response = await this.store.findRecord('detailed-analysis', id); - this.screenshots = response.screenshots || []; - - this.loadCurrentImage(); - this.preloadNextImage(); + try { + const response = await this.store.findRecord('detailed-analysis', id); + this.screenshots = response.screenshots || []; + this.currentIndex = 0; + this.updateLoadingStateForCurrentImage(); + this.loadCurrentImage(); + this.preloadNextImage(); + } catch { + this.screenshots = []; + this.currentIndex = 0; + this.isLoadingImage = false; + this.imageLoadError = true; + } });
🧹 Nitpick comments (2)
app/components/file-details/screenshots/index.scss (1)
34-46: Consider whether!importantis necessary.Line 44 uses
!importantto force background color on hover and disabled states. If this is overriding a framework default that's difficult to target otherwise, it's acceptable. However, if specificity can be increased through selector composition, prefer that approach.app/components/file-details/screenshots/index.ts (1)
197-200: Consider explicitly canceling the task on destroy.While
cleanupAllImages()properly removes image event listeners, theloadScreenshotstask is not explicitly canceled. Ember-concurrency tasks with default concurrency may continue running after component destruction.Consider adding task cancellation:
willDestroy() { super.willDestroy(); + this.loadScreenshots.cancelAll(); this.cleanupAllImages(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/adapters/detailed-analysis.ts(1 hunks)app/components/file-details/screenshots/index.hbs(1 hunks)app/components/file-details/screenshots/index.scss(1 hunks)app/components/file-details/screenshots/index.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.scss(2 hunks)app/components/file-details/vulnerability-analysis-details/index.hbs(8 hunks)app/components/file-details/vulnerability-analysis-details/index.scss(1 hunks)tests/integration/components/file-details/screenshots/index-test.js(1 hunks)tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js
🚧 Files skipped from review as they are similar to previous changes (7)
- app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss
- tests/integration/components/file-details/screenshots/index-test.js
- app/components/file-details/screenshots/index.hbs
- app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs
- app/adapters/detailed-analysis.ts
- app/components/file-details/vulnerability-analysis-details/findings/index.hbs
- app/components/file-details/vulnerability-analysis-details/findings/index.scss
🧰 Additional context used
🧬 Code graph analysis (1)
app/components/file-details/screenshots/index.ts (1)
app/models/analysis.ts (1)
AnalysisModel(45-291)
🔇 Additional comments (7)
app/components/file-details/vulnerability-analysis-details/index.scss (1)
58-58: LGTM! Typo corrected.The selector has been correctly updated from
analysis-overridded-passedtoanalysis-overridden-passed, fixing the spelling error and ensuring consistency with the updated class names used in templates.app/components/file-details/screenshots/index.scss (2)
64-68: LGTM! Proper responsive image constraints.The combination of
max-width: 100%,max-height: 500px, andobject-fit: containensures images scale responsively while preserving aspect ratio and preventing overflow.
70-81: LGTM! Clean utility classes.The
ak-display-noneutility andloading-containerwith flexbox centering are straightforward and properly match the container's min-height for consistent layout.app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss (1)
7-7: LGTM! Typo corrected.The selector has been correctly updated to
analysis-overridden-passed, consistent with the fixes applied across other stylesheets in this PR.app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs (1)
4-4: LGTM! Template class name corrected.The conditional class binding now uses
"analysis-overridden-passed", ensuring it matches the updated CSS selector and maintains consistency across the codebase.app/components/file-details/vulnerability-analysis-details/index.hbs (2)
66-66: LGTM! All class name typos corrected.All eight occurrences of
"analysis-overridded-passed"have been correctly updated to"analysis-overridden-passed", ensuring template class bindings match the updated CSS selectors.Also applies to: 102-102, 127-127, 168-168, 197-197, 226-226, 280-280, 310-310
274-274: LGTM! Screenshots component properly integrated.The new
<FileDetails::Screenshots>component is correctly integrated with the analysis model passed as a prop. The placement within the vulnerability analysis details flow is appropriate.
avzz-19
force-pushed
the
dast-screenshots
branch
from
eed71ab to
444f8cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/components/file-details/screenshots/index.ts (3)
30-34: Handle promise rejection to prevent unhandled errors.The constructor calls
loadScreenshots.perform()without error handling. If the task fails (network error, invalid ID, etc.), the promise rejection will be unhandled.This issue was previously flagged. Consider adding
.catch()to handle errors:constructor(owner: unknown, args: FileDetailsScreenshotsSignature['Args']) { super(owner, args); - this.loadScreenshots.perform(this.analysis.id); + this.loadScreenshots.perform(this.analysis.id).catch(() => { + // Task errors are already handled within the task + }); }
77-91: Clear loading state when no current screenshot exists.The early return at line 80 leaves
isLoadingImageastruewhen there's no current screenshot, causing a perpetual loading spinner.Apply this diff:
@action updateLoadingStateForCurrentImage() { if (!this.currentScreenshot) { + this.isLoadingImage = false; + this.imageLoadError = false; return; }
183-194: Add error handling and state reset in loadScreenshots task.The task doesn't catch errors from
store.findRecord(), and doesn't resetcurrentIndexwhen loading new data, which could point past the array bounds.Apply this diff:
loadScreenshots = task(async (id?: string) => { // If no id is provided, do nothing if (!id) { + this.screenshots = []; + this.currentIndex = 0; + this.isLoadingImage = false; return; } - const response = await this.store.findRecord('detailed-analysis', id); - this.screenshots = response.screenshots || []; - - this.loadCurrentImage(); - this.preloadNextImage(); + try { + const response = await this.store.findRecord('detailed-analysis', id); + this.screenshots = response.screenshots || []; + this.currentIndex = 0; + this.updateLoadingStateForCurrentImage(); + this.loadCurrentImage(); + this.preloadNextImage(); + } catch { + this.screenshots = []; + this.currentIndex = 0; + this.isLoadingImage = false; + this.imageLoadError = true; + } });
🧹 Nitpick comments (1)
app/models/detailed-analysis.ts (1)
3-6: Add a default value for the screenshots array attribute.The
screenshotsattribute should have a default value to preventundefinedwhen the API doesn't return the field. This ensures safer access patterns throughout the codebase.Apply this diff:
export default class DetailedAnalysisModel extends Model { - @attr() + @attr('array', { defaultValue: () => [] }) declare screenshots: string[]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
app/adapters/detailed-analysis.ts(1 hunks)app/components/file-details/screenshots/index.hbs(1 hunks)app/components/file-details/screenshots/index.scss(1 hunks)app/components/file-details/screenshots/index.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.scss(2 hunks)app/components/file-details/vulnerability-analysis-details/index.hbs(8 hunks)app/components/file-details/vulnerability-analysis-details/index.scss(1 hunks)app/models/detailed-analysis.ts(1 hunks)app/styles/_component-variables.scss(1 hunks)mirage/factories/detailed-analysis.ts(1 hunks)mirage/models/detailed-analysis.ts(1 hunks)tests/integration/components/file-details/screenshots/index-test.js(1 hunks)tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js(2 hunks)translations/en.json(1 hunks)translations/ja.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs
- app/components/file-details/vulnerability-analysis-details/index.hbs
- app/components/file-details/vulnerability-analysis-details/findings/index.hbs
- app/components/file-details/screenshots/index.hbs
- tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js
- app/components/file-details/vulnerability-analysis-details/index.scss
- mirage/models/detailed-analysis.ts
- app/components/file-details/screenshots/index.scss
- mirage/factories/detailed-analysis.ts
- app/adapters/detailed-analysis.ts
- translations/ja.json
- app/styles/_component-variables.scss
🧰 Additional context used
🧬 Code graph analysis (2)
app/components/file-details/screenshots/index.ts (1)
app/models/analysis.ts (1)
AnalysisModel(45-291)
tests/integration/components/file-details/screenshots/index-test.js (1)
app/components/file-details/screenshots/index.ts (1)
analysis(36-38)
⏰ 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). (2)
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
🔇 Additional comments (6)
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss (1)
7-7: LGTM! Typo corrected.The class name has been correctly updated from "analysis-overridded-passed" to "analysis-overridden-passed".
app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs (1)
5-5: LGTM! Template class name corrected.The conditional class name has been updated to match the corrected CSS selector.
app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss (1)
50-56: LGTM! CSS selector corrected.The selector has been updated from
.analysis-overridded-passedto.analysis-overridden-passed, maintaining consistency with the class name fixes across the codebase.app/components/file-details/vulnerability-analysis-details/findings/index.scss (1)
7-7: LGTM! Selector naming corrected.The nested selector has been properly updated to
&.analysis-overridden-passed, fixing the typo consistently with other files in this PR.translations/en.json (1)
1783-1783: LGTM! Translation key added correctly.The new "screenshots" translation key is appropriately named and placed alphabetically in the translation file.
tests/integration/components/file-details/screenshots/index-test.js (1)
48-95: LGTM! Comprehensive navigation tests.The test properly verifies screenshot rendering, navigation controls, counter display with translation, and image src updates when navigating. Good coverage of the component's core functionality.
tests/integration/components/file-details/screenshots/index-test.js
Outdated
Show resolved
Hide resolved
avzz-19
force-pushed
the
dast-screenshots
branch
from
444f8cf to
8d2b756
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
translations/ja.json (1)
1783-1784: Localize JA value for screenshots.Use a Japanese string instead of English.
- "screenshots": "Screenshots", + "screenshots": "スクリーンショット",app/components/file-details/screenshots/index.ts (2)
78-92: Fix spinner stuck when there’s no current image.Early return keeps isLoadingImage true for empty lists.
@action updateLoadingStateForCurrentImage() { - if (!this.currentScreenshot) { - return; - } + if (!this.currentScreenshot) { + this.isLoadingImage = false; + this.imageLoadError = false; + return; + } // Only update loading state based on current image if (this.loadedImages.has(this.currentScreenshot)) { this.isLoadingImage = false; this.imageLoadError = false; } else { this.isLoadingImage = true; this.imageLoadError = false; } }
184-202: Harden loadScreenshots: handle errors and reset state.Catch network errors; handle missing id; reset index/state; refresh loading state for new data.
- loadScreenshots = task(async (id?: string) => { + loadScreenshots = task(async (id?: string) => { // If no id is provided, do nothing if (!id) { - return; + this.screenshots = []; + this.currentIndex = 0; + this.isLoadingImage = false; + this.imageLoadError = false; + return []; } - const adapter = this.store.adapterFor('detailed-analysis'); + const adapter = this.store.adapterFor('detailed-analysis'); adapter.setNestedUrlNamespace(String(id)); - const response = (await this.store.queryRecord( - 'detailed-analysis', - {} - )) as DetailedAnalysisModel; - - this.screenshots = response.screenshots || []; - - this.loadCurrentImage(); - this.preloadNextImage(); + try { + const response = (await this.store.queryRecord( + 'detailed-analysis', + {} + )) as DetailedAnalysisModel; + this.screenshots = response.screenshots || []; + // reset/clamp index for fresh data + this.currentIndex = 0; + this.updateLoadingStateForCurrentImage(); + this.loadCurrentImage(); + this.preloadNextImage(); + return this.screenshots; + } catch { + this.screenshots = []; + this.currentIndex = 0; + this.isLoadingImage = false; + this.imageLoadError = true; + return []; + } });
🧹 Nitpick comments (3)
app/components/file-details/screenshots/index.ts (1)
31-35: Optionally guard constructor call and swallow rejections.Avoid unhandled rejections if the task throws.
constructor(owner: unknown, args: FileDetailsScreenshotsSignature['Args']) { super(owner, args); - this.loadScreenshots.perform(this.analysis.id); + if (this.analysis?.id) { + void this.loadScreenshots.perform(this.analysis.id); + } else { + this.isLoadingImage = false; + } }tests/acceptance/breadcrumbs/scan-details-test.js (1)
64-70: DRY the detailed-analysis Mirage stub across tests.Extract this route to a shared helper (e.g., setupRequiredEndpoints or a new setupDetailedAnalysisEndpoints fn) to avoid duplication.
tests/acceptance/file-details-test.js (1)
226-232: Deduplicate the detailed-analysis stub.Reuse a common test helper for /v2/analyses/:id/detailed-analysis across suites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
app/adapters/detailed-analysis.ts(1 hunks)app/components/file-details/screenshots/index.hbs(1 hunks)app/components/file-details/screenshots/index.scss(1 hunks)app/components/file-details/screenshots/index.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.scss(2 hunks)app/components/file-details/vulnerability-analysis-details/index.hbs(8 hunks)app/components/file-details/vulnerability-analysis-details/index.scss(1 hunks)app/models/detailed-analysis.ts(1 hunks)app/styles/_component-variables.scss(1 hunks)mirage/factories/detailed-analysis.ts(1 hunks)mirage/models/detailed-analysis.ts(1 hunks)tests/acceptance/breadcrumbs/scan-details-test.js(1 hunks)tests/acceptance/file-details-test.js(1 hunks)tests/integration/components/file-details/screenshots/index-test.js(1 hunks)tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js(2 hunks)tests/integration/components/file-details/vulnerability-analysis-details/index-test.js(1 hunks)translations/en.json(1 hunks)translations/ja.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/components/file-details/vulnerability-analysis-details/index.scss
- app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs
🚧 Files skipped from review as they are similar to previous changes (12)
- app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss
- app/models/detailed-analysis.ts
- tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js
- tests/integration/components/file-details/screenshots/index-test.js
- mirage/models/detailed-analysis.ts
- app/components/file-details/vulnerability-analysis-details/findings/index.scss
- app/components/file-details/screenshots/index.hbs
- app/components/file-details/screenshots/index.scss
- app/styles/_component-variables.scss
- app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs
- app/components/file-details/vulnerability-analysis-details/index.hbs
- app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss
🧰 Additional context used
🧬 Code graph analysis (2)
app/components/file-details/screenshots/index.ts (2)
app/models/analysis.ts (1)
AnalysisModel(45-291)app/models/detailed-analysis.ts (1)
DetailedAnalysisModel(3-6)
app/adapters/detailed-analysis.ts (1)
app/adapters/commondrf-nested.ts (1)
CommondrfNestedAdapter(5-30)
⏰ 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). (2)
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
🔇 Additional comments (5)
translations/en.json (1)
1783-1783: LGTM for EN key.Key is correctly added and matches usage.
app/components/file-details/vulnerability-analysis-details/findings/index.hbs (1)
3-6: Typo fix verified as complete—no stale references found.The search confirmed the old misspelled class
analysis-overridded-passed(double 'd') has been completely removed from the codebase. The new correct classanalysis-overridden-passed(single 'd') is properly defined across all relevant SCSS files and used consistently in 16+ locations, including templates and tests. The refactor is complete and correct.tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (1)
108-116: LGTM!The Mirage route mock correctly returns the expected detailed-analysis data structure for testing purposes.
mirage/factories/detailed-analysis.ts (1)
6-12: LGTM!The screenshots generation logic correctly uses a function that returns an array of faker-generated image URLs.
app/adapters/detailed-analysis.ts (1)
8-10: LGTM!The method correctly uses
this.namespace_v2as requested in previous review feedback.
avzz-19
force-pushed
the
dast-screenshots
branch
from
8d2b756 to
bed8f13
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
app/components/file-details/screenshots/index.ts (2)
184-202: Harden data load: handle errors, reset state, and clamp index for new data.Prevents stuck UI on failures and out-of-bounds indices.
- loadScreenshots = task(async (id?: string) => { + loadScreenshots = task(async (id?: string) => { // If no id is provided, do nothing if (!id) { - return; + this.screenshots = []; + this.currentIndex = 0; + this.isLoadingImage = false; + this.imageLoadError = false; + return []; } - const adapter = this.store.adapterFor('detailed-analysis'); - adapter.setNestedUrlNamespace(String(id)); - - const response = (await this.store.queryRecord( - 'detailed-analysis', - {} - )) as DetailedAnalysisModel; - - this.screenshots = response.screenshots || []; - - this.loadCurrentImage(); - this.preloadNextImage(); + try { + const adapter = this.store.adapterFor('detailed-analysis'); + adapter.setNestedUrlNamespace(String(id)); + const response = (await this.store.queryRecord( + 'detailed-analysis', + {} + )) as DetailedAnalysisModel; + this.screenshots = response.screenshots || []; + this.currentIndex = 0; + this.updateLoadingStateForCurrentImage(); + this.loadCurrentImage(); + this.preloadNextImage(); + return this.screenshots; + } catch { + this.screenshots = []; + this.currentIndex = 0; + this.isLoadingImage = false; + this.imageLoadError = true; + return []; + } });
78-92: Fix empty-list spinner: clear loading when there’s no current image.Otherwise the loader can spin forever with 0 screenshots.
@action updateLoadingStateForCurrentImage() { - if (!this.currentScreenshot) { - return; - } + if (!this.currentScreenshot) { + this.isLoadingImage = false; + this.imageLoadError = false; + return; + }translations/ja.json (1)
1783-1783: Localize the new key in JA.Replace the English value with Japanese.
- "screenshots": "Screenshots", + "screenshots": "スクリーンショット",
🧹 Nitpick comments (4)
app/models/detailed-analysis.ts (1)
4-5: Type the attr and set a safe default.Prevents undefined and clarifies TS type.
-export default class DetailedAnalysisModel extends Model { - @attr() - declare screenshots: string[]; -} +export default class DetailedAnalysisModel extends Model { + @attr<string[]>({ defaultValue: () => [] }) + declare screenshots: string[]; +}app/adapters/detailed-analysis.ts (1)
8-10: Avoid mutable adapter.namespace per request; derive URL from inputs to prevent races.Parallel requests can clobber namespace; consider building URL from id in buildURL/urlForQueryRecord instead of mutating state.
- setNestedUrlNamespace(id: string) { - this.namespace = `${this.namespace_v2}/analyses/${id}`; - } + setNestedUrlNamespace(id: string): void { + this.namespace = `${this.namespace_v2}/analyses/${id}`; + }Alternatively, pass id in query and compute URL in buildURL without mutating namespace.
app/components/file-details/screenshots/index.ts (2)
31-35: Guard constructor and avoid unhandled rejections.Don’t perform when id is missing; clear loading state accordingly.
constructor(owner: unknown, args: FileDetailsScreenshotsSignature['Args']) { super(owner, args); - - this.loadScreenshots.perform(this.analysis.id); + const id = this.analysis?.id; + if (id) { + void this.loadScreenshots.perform(id); + } else { + this.screenshots = []; + this.isLoadingImage = false; + this.imageLoadError = false; + } }
59-76: Optionally trigger load for the newly selected image.Idempotent and safer if preloading missed; keeps UX snappy.
@action handleNextClick() { if (this.hasNext) { this.currentIndex += 1; this.updateLoadingStateForCurrentImage(); + this.loadCurrentImage(); this.preloadNextImage(); } } @@ @action handlePrevClick() { if (this.hasPrevious) { this.currentIndex -= 1; this.updateLoadingStateForCurrentImage(); + this.loadCurrentImage(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
app/adapters/detailed-analysis.ts(1 hunks)app/components/file-details/screenshots/index.hbs(1 hunks)app/components/file-details/screenshots/index.scss(1 hunks)app/components/file-details/screenshots/index.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.scss(2 hunks)app/components/file-details/vulnerability-analysis-details/index.hbs(8 hunks)app/components/file-details/vulnerability-analysis-details/index.scss(1 hunks)app/models/detailed-analysis.ts(1 hunks)app/styles/_component-variables.scss(1 hunks)mirage/factories/detailed-analysis.ts(1 hunks)mirage/models/detailed-analysis.ts(1 hunks)tests/acceptance/breadcrumbs/scan-details-test.js(1 hunks)tests/acceptance/file-details-test.js(1 hunks)tests/integration/components/file-details/screenshots/index-test.js(1 hunks)tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js(2 hunks)tests/integration/components/file-details/vulnerability-analysis-details/index-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 (13)
- mirage/models/detailed-analysis.ts
- translations/en.json
- tests/integration/components/file-details/vulnerability-analysis-details/index-test.js
- mirage/factories/detailed-analysis.ts
- app/components/file-details/screenshots/index.scss
- tests/acceptance/breadcrumbs/scan-details-test.js
- app/components/file-details/vulnerability-analysis-details/index.hbs
- tests/integration/components/file-details/screenshots/index-test.js
- app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs
- app/components/file-details/vulnerability-analysis-details/index.scss
- app/components/file-details/vulnerability-analysis-details/findings/index.scss
- tests/acceptance/file-details-test.js
- app/components/file-details/screenshots/index.hbs
🧰 Additional context used
🧬 Code graph analysis (2)
app/adapters/detailed-analysis.ts (1)
app/adapters/commondrf-nested.ts (1)
CommondrfNestedAdapter(5-30)
app/components/file-details/screenshots/index.ts (2)
app/models/analysis.ts (1)
AnalysisModel(45-291)app/models/detailed-analysis.ts (1)
DetailedAnalysisModel(3-6)
⏰ 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). (2)
- GitHub Check: Setup & Build Application
- GitHub Check: Publish to Cloudflare Pages
🔇 Additional comments (7)
app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs (1)
5-5: LGTM! Typo corrected.The class name has been corrected from "analysis-overridded-passed" to "analysis-overridden-passed" ("overridden" is the correct past participle).
tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js (1)
19-19: LGTM! Test expectations updated to match the corrected class name.The test description and assertions have been updated to reference "analysis-overridden-passed" consistently.
Also applies to: 76-76, 80-80
app/components/file-details/vulnerability-analysis-details/findings/index.hbs (1)
5-5: LGTM! Consistent typo correction.The class name has been corrected to "analysis-overridden-passed", maintaining consistency across the vulnerability analysis components.
app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss (1)
50-56: LGTM! CSS selector updated to match the corrected class name.The selector has been updated from
.analysis-overridded-passedto.analysis-overridden-passed, ensuring styles are applied correctly.app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss (1)
7-7: LGTM! SCSS selector corrected.The nested selector has been updated to
&.analysis-overridden-passed, maintaining consistency with the class name corrections across the codebase.app/styles/_component-variables.scss (1)
1187-1195: LGTM — tokens are consistent with existing file-details vars.No issues; theme inheritance looks fine.
app/adapters/detailed-analysis.ts (1)
4-6: ****The delegation pattern is confirmed and working as-is. The codebase inherits from
DRFAdapter(viaDRFAuthenticationBase), which implements the standard Ember Data pattern wherebuildURL()delegates to the_buildURL()hook. Overriding_buildURL()is the correct approach here—30+ adapters throughout the codebase successfully use this same pattern. The suggested change to overridebuildURL()instead would contradict the established convention and is unnecessary.Likely an incorrect or invalid review comment.
avzz-19
force-pushed
the
dast-screenshots
branch
from
bed8f13 to
81b729e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/acceptance/file-details-test.js (1)
62-67: Unnecessary async/await in Mirage seeding
this.server.create('detailed-analysis', ...)is synchronous, so wrapping it inasync/awaitinsidemapdoesn’t provide any benefit and slightly obscures intent. A simpler version is:analyses.forEach((analysis) => { this.server.create('detailed-analysis', { id: analysis.id }); });Functionally identical, but clearer.
mirage/factories/detailed-analysis.ts (1)
4-23: Factory data shape looks good; only minor nitsThe factory structure (three findings with optional
screenshotURL) lines up withDetailedFindingand should work well with Mirage and the new model. If you touch this again, you could optionally:
- Simplify the id expression to
i + 101.- Rename
descto something likefindingsfor clarity.Both are purely cosmetic; behavior is fine as-is.
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss (1)
7-112: Styles align with new behavior; consider making screenshot container explicitly flexThe updated overridden state and new
.vulnerability-finding-row/.vulnerability-finding-containerstyles look aligned with the detailed findings UI and are consistent with the existing token usage.One small robustness tweak:
.screenshot-containerusesalign-itemsandjustify-content, but does not setdisplay. If a parent does not already make this element a flex container, centering won’t take effect. If you intend the container itself to control centering, consider:.screenshot-container { - width: 500px; - align-items: center; - justify-content: center; + width: 500px; + display: flex; + align-items: center; + justify-content: center; }Otherwise (if a parent already handles flex layout), current code is fine.
📜 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 (26)
app/adapters/detailed-analysis.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs(3 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss(3 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.hbs(2 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.scss(2 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.ts(3 hunks)app/components/file-details/vulnerability-analysis-details/index.hbs(8 hunks)app/components/file-details/vulnerability-analysis-details/index.scss(1 hunks)app/models/detailed-analysis.ts(1 hunks)app/styles/_component-variables.scss(1 hunks)app/utils/icons.ts(1 hunks)mirage/factories/detailed-analysis.ts(1 hunks)mirage/models/detailed-analysis.ts(1 hunks)package.json(1 hunks)public/ak-icons.json(1 hunks)scripts/build-icons.mjs(4 hunks)tests/acceptance/breadcrumbs/scan-details-test.js(1 hunks)tests/acceptance/file-details-test.js(2 hunks)tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js(2 hunks)tests/integration/components/file-details/vulnerability-analysis-details/index-test.js(8 hunks)translations/en.json(1 hunks)translations/ja.json(1 hunks)types/ak-icons.d.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/integration/components/file-details/vulnerability-analysis-details/index-test.js
- translations/en.json
- app/adapters/detailed-analysis.ts
- mirage/models/detailed-analysis.ts
- app/components/file-details/vulnerability-analysis-details/index.scss
- translations/ja.json
- app/components/file-details/vulnerability-analysis-details/index.hbs
- app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs
🧰 Additional context used
🧬 Code graph analysis (5)
scripts/build-icons.mjs (1)
app/utils/icons.ts (1)
IconoirIconsSet(196-196)
types/ak-icons.d.ts (1)
app/utils/icons.ts (1)
IconoirIconsSet(196-196)
app/components/file-details/vulnerability-analysis-details/findings/index.ts (3)
app/models/detailed-analysis.ts (2)
DetailedFinding(3-7)DetailedAnalysisModel(9-12)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
customVulnerableFindings(35-37)app/utils/parse-vulnerable-api-finding.ts (2)
isVulnerableApiFinding(75-90)parseVulnerableApiFinding(97-115)
tests/acceptance/file-details-test.js (2)
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (14)
analyses(86-96)analysis(280-280)analysis(364-364)analysis(446-446)analysis(528-528)analysis(596-596)analysis(694-694)analysis(879-879)analysis(923-923)analysis(979-979)analysis(1042-1042)analysis(1159-1159)analysis(1290-1290)analysis(1408-1421)app/components/file-details/dynamic-scan/header/index.ts (1)
analyses(45-47)
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
app/models/detailed-analysis.ts (1)
DetailedFinding(3-7)
⏰ 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: Setup & Build Application
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Cloudflare Pages
🔇 Additional comments (16)
app/styles/_component-variables.scss (1)
1187-1194: Screenshots design tokens look consistentNew
--file-details-screenshots-*variables align with existing file-details tokens and reuse existing color and radius tokens appropriately. No issues spotted.app/components/file-details/vulnerability-analysis-details/findings/index.scss (1)
7-26: Passed-state and pagination styles are coherent
.analysis-section.analysis-overridden-passedcorrectly switches to the “marked-passed” tokens, and the pagination hover/disabled rule keeps button background stable. Styling changes look consistent with the rest of the file.Also applies to: 71-76
app/utils/icons.ts (1)
192-196: Iconoir icon set export is consistent
IconoirIconsSet = ['screenshot'] as constmatches the existing pattern for other icon sets and should integrate cleanly with the icon build/types pipeline.tests/acceptance/breadcrumbs/scan-details-test.js (1)
64-80: Mirage mock for detailed-analysis endpoint is adequateThe new
/v2/analyses/:id/detailed-analysishandler returns a minimal but correctly shaped payload (id + findings with title/description/screenshot), which is enough to unblock the breadcrumb and details flows in this test. No issues from a test-stability perspective.package.json (1)
59-69: @iconify-json/iconoir dependency properly integrated and actively usedVerification confirms the package is correctly integrated:
- Imported and consumed by
scripts/build-icons.mjs- Types defined in
types/ak-icons.d.tswith icon union type- Build output generated in
public/ak-icons.jsonwith correct prefix- Icon set exported from
app/utils/icons.tsThe dependency is ready and the build pipeline includes it as expected.
app/models/detailed-analysis.ts (1)
1-18: The review comment is incorrect and should be disregarded.The code follows established patterns across the codebase:
Module registry path is consistent: All 100+ models in
app/models/use the identical module specifier'ember-data/types/registries/model'. This is not an inconsistency—it's the standard throughout the codebase.Array attributes without defaults is the codebase convention: Other models like
api-scan-options.tsdeclare array attributes with bare@attr(e.g.,@attr declare dsApiCaptureFilters: string[];). The codebase does not use@attr({ defaultValue: () => [] })for array types; defaults are reserved for primitives only. Thedetailed-analysis.tsmodel follows this exact established pattern.No changes are needed.
Likely an incorrect or invalid review comment.
tests/acceptance/file-details-test.js (1)
232-234: Code verified as correctThe Mirage route implementation at lines 232-234 is verified to be correct:
- Model name 'detailed-analysis' correctly maps to
schema.detailedAnalysesper Mirage.js conventions- Seeding pattern at line 63 matches the route implementation
- The
.toJSON()call and optional chaining align with other routes in the file (lines 96-98, 104-106, 133-134)- No extra boilerplate needed
app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss (1)
50-56: Class rename aligns styles with templates/testsThe
.analysis-overridden-passedselector correctly matches the updated class name used in the HBS and tests; styling remains consistent for the “passed” state.tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js (1)
19-20: Tests correctly track the renamedanalysis-overridden-passedclassThe parameterized test now asserts the correct class for both
markedAsPassedcases, staying in sync with the component and SCSS rename.Also applies to: 73-81
app/components/file-details/vulnerability-analysis-details/findings/index.hbs (1)
3-6: Good alignment of class rename and loading state bindingThe conditional
analysis-overridden-passedclass now matches the shared naming across styles and tests, and wiring@isLoading={{this.loadDetailedAnalyses.isRunning}}intoCustomVulnerabilitiesgives that component a clear, single source of truth for the detailed-analysis loading state (assuming the task exists on the backing class).If not already done, please confirm that
this.loadDetailedAnalysesis always defined where this template is used (e.g., a task from the corresponding component class) soisRunningis safe to access.Also applies to: 104-109
scripts/build-icons.mjs (1)
13-14: Iconoir icon set wiring matches existing icon collectionsThe new Iconoir imports,
iconoirCollectioncreation, and inclusion in theoutputobject follow the same pattern as the other icon sets, so the build script remains consistent and maintainable.Please double-check that:
@iconify-json/iconoiris present indevDependencies, and- every name in
IconoirIconsSet(currentlyscreenshot) exists in that package, by running the icon build script once and confirmingiconoirCollectionappears correctly inpublic/ak-icons.json.Also applies to: 25-26, 41-55
types/ak-icons.d.ts (1)
11-12: Type surface cleanly extended for Iconoir iconsThe new
IconoirIconalias, inclusion inAkIconsSet, and theiconoir:${IconoirIcon}variant keep the icon type system coherent and in line with existing sets.Please confirm that
IconoirIconsSetexported fromirene/utils/iconsstays in sync with the Iconoir entries used inscripts/build-icons.mjsto avoid type/build drift if more Iconoir icons are added later.Also applies to: 23-24, 35-36, 47-48, 61-62
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs (1)
1-8: Custom findings layout and class rename look consistentUsing
analysis-overridden-passedon the wrapper and switching the loop tothis.customVulnerableFindingsmake sense given the enriched data and keep this component aligned with the rest of the vulnerability-analysis UI. The row layout withvulnerability-finding-containerand the conditional screenshot button is straightforward and testable via the addeddata-test-*attributes.Please verify that the backing component class exposes
customVulnerableFindings(derived from@customVulnerableFindingsand any detailed-analysis data) and definesopenScreenshotModalto setthis.screenshotandthis.screenshotDrawerOpen, so these template bindings are always valid.Also applies to: 24-60
app/components/file-details/vulnerability-analysis-details/findings/index.ts (1)
26-35: Tracked state and type wiring look consistentThe new tracked fields (
customVulnerableFindings: DetailedFinding[],detailedAnalysis: DetailedAnalysisModel | null) plus thestoreinjection are consistent with the new detailed-analysis model and should make the data flow to the template straightforward.app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
22-51: Screenshot drawer state management looks cleanThe component signature and tracked state (
customVulnerableFindings: DetailedFinding[],screenshotDrawerOpen,screenshot) are well-typed and theopenScreenshotModal/closeScreenshotModalactions give a simple, predictable API to the template. No issues from here.public/ak-icons.json (1)
640-649: New iconoirscreenshoticon entry is consistentThe
iconoirCollectionblock (prefix,icons.screenshot.body,lastModified,width,height) is shaped consistently with the other collections in this file, so it should integrate cleanly with the existing icon loading/build pipeline.
...onents/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs
Show resolved
Hide resolved
app/components/file-details/vulnerability-analysis-details/findings/index.ts
Show resolved
Hide resolved
avzz-19
force-pushed
the
dast-screenshots
branch
from
81b729e to
a1f1aa8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (1)
855-877: UsenewAnalysisdirectly instead of relying onthis.file.analyses.at(-1)The test explicitly creates
newAnalysiswith specific attachments and seeds a matchingdetailed-analysis, but then renders usingthis.file.analyses.at(-1). This relies on Ember Data's store automatically updating the file's analyses collection, which is not guaranteed when the file record was already loaded inbeforeEach.Replace at line 879-881:
const analysis = this.file.analyses.at(-1); this.set('analysis', analysis);With:
this.set('analysis', newAnalysis);This ensures the component is wired to the exact analysis with the seeded attachments and detailed findings.
Also applies to lines 879-882.
♻️ Duplicate comments (2)
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs (1)
85-112: Fixlocal-classusage and remove redundant{{#if this.screenshot}}Two issues in the screenshot drawer are still unresolved:
local-classmust be static for CSS modules. Interpolating{{if this.imageLoading "ak-display-none"}}insidelocal-classis not supported and will not reliably apply the visibility class.- The inner
{{#if this.screenshot}}is redundant because the outer block already guards onthis.screenshot.A minimal fix is:
{{#if this.screenshot}} <AkStack @alignItems='center' @justifyContent='center' class='p-2' local-class='screenshot-container' > - {{#if this.screenshot}} - {{#if this.imageLoading}} - <div local-class='loading-container'> - <AkStack @alignItems='center' @spacing='2'> - <AkLoader @size={{16}} /> - <AkTypography>{{t "loading"}}...</AkTypography> - </AkStack> - </div> - {{/if}} - - <img - src={{this.screenshot}} - alt={{t 'screenshots'}} - local-class='screenshot-image {{if - this.imageLoading - "ak-display-none" - }}' - {{on 'load' this.onScreenshotLoad}} - data-test-analysisDetails-vulFinding-screenshotImage - /> - {{/if}} + {{#if this.imageLoading}} + <div local-class='loading-container'> + <AkStack @alignItems='center' @spacing='2'> + <AkLoader @size={{16}} /> + <AkTypography>{{t "loading"}}...</AkTypography> + </AkStack> + </div> + {{/if}} + + <img + src={{this.screenshot}} + alt={{t "screenshots"}} + local-class='screenshot-image' + class={{if this.imageLoading "ak-display-none"}} + {{on 'load' this.onScreenshotLoad}} + data-test-analysisDetails-vulFinding-screenshotImage + /> </AkStack> {{/if}}This keeps CSS-module classes static and moves conditional visibility to the regular
classattribute while simplifying the template.app/components/file-details/vulnerability-analysis-details/findings/index.ts (1)
84-108: ReplacepushObjectswith native array push to avoid runtime error
vulnerableApiFindingsis a plain JS array:const vulnerableApiFindings: VulnerableApiFinding[] = [];Calling
vulnerableApiFindings.pushObjects(...)will throw at runtime becausepushObjectsis an Ember Array method, not available on native arrays.Use native
pushwith spread instead:- this.analysis.findings.forEach((finding) => { - const content = finding.description; - - if (isVulnerableApiFinding(content)) { - vulnerableApiFindings.pushObjects(parseVulnerableApiFinding(content)); - } - }); + this.analysis.findings.forEach((finding) => { + const content = finding.description; + + if (isVulnerableApiFinding(content)) { + vulnerableApiFindings.push( + ...parseVulnerableApiFinding(content) + ); + } + });The rest of the task (awaiting
loadDetailedAnalyses, then populatingcustomVulnerableFindingsfrom non-API detailed findings) can remain as-is.
🧹 Nitpick comments (4)
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (2)
98-118: Make screenshot seeding deterministic for tests
hasScreenshot = faker.datatype.boolean()means some runs will create no screenshots for any finding, so the screenshot button/drawer path might not be exercised even though assertions are present. For stable coverage, prefer a deterministic rule per finding (e.g., first finding gets a screenshot, the rest don’t):- analyses.forEach((analysis) => { - const sourceFindings = - analysis.findings ?? - (analysis.attrs && analysis.attrs.findings) ?? - []; - - const hasScreenshot = faker.datatype.boolean(); - - const detailedFindings = sourceFindings.map((f) => ({ - title: f.title ?? null, - description: f.description ?? '', - screenshot: hasScreenshot - ? faker.image.urlLoremFlickr({ category: 'technology' }) - : '', - })); + analyses.forEach((analysis) => { + const sourceFindings = + analysis.findings ?? + (analysis.attrs && analysis.attrs.findings) ?? + []; + + const detailedFindings = sourceFindings.map((f, index) => ({ + title: f.title ?? null, + description: f.description ?? '', + screenshot: + index === 0 + ? faker.image.urlLoremFlickr({ category: 'technology' }) + : '', + }));This still uses realistic data while guaranteeing that both “with screenshot” and “without screenshot” branches are always test-covered.
Also applies to: 130-132
1423-1431: Per-test detailed-analysis seeding is correct but could be centralizedThe repeated pattern:
const detailedFindings = analysis.findings.map((f) => ({ title: f.title ?? null, description: f.description ?? '', })); this.server.create('detailed-analysis', { id: analysis.id, findings: detailedFindings, });correctly mirrors
analysis.findingsintodetailed-analysisfor various scenarios (regulatory content, empty-regulatory, and “more details” tests). If this pattern grows further, consider extracting a small helper (e.g.,createDetailedAnalysisFor(this.server, analysis)) to keep the tests DRY and easier to maintain, but it’s not urgent.Also applies to: 1570-1578, 1851-1860
app/components/file-details/vulnerability-analysis-details/findings/index.ts (1)
110-123: Be aware thatsetNestedUrlNamespacemutates a shared adapter
loadDetailedAnalysesmutates thedetailed-analysisadapter before issuing aqueryRecord:const adapter = this.store.adapterFor('detailed-analysis'); adapter.setNestedUrlNamespace(String(id)); this.detailedAnalysis = (await this.store.queryRecord( 'detailed-analysis', {} )) as DetailedAnalysisModel;This is fine as long as only one analysis’s detailed findings are loaded at a time, but if multiple
FileDetails::VulnerabilityAnalysisDetails::Findingsinstances ever coexist (each with a differentanalysis.id), their requests could race on the same adapter instance.Not urgent now, but consider, longer term, either:
- Making
setNestedUrlNamespacea pure helper that returns a URL, or- Passing the
analysis.idvia adapter options/URL params instead of adapter-wide mutable state.app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
39-63: Tighten screenshot modal state reset and loading behavior (optional)The screenshot modal actions are generally solid; a couple of small improvements you might consider:
- If
title/descriptionare intended to reflect the currently opened finding, you may want to clear them incloseScreenshotModal()alongsidescreenshot/imageLoadingto avoid stale state being rendered if the template references them.- Ensure
imageLoadingis also reset on image load failure; one simple pattern is to wire this sameonScreenshotLoadaction to both theloadanderrorevents of the<img>so the spinner cannot get stuck if the screenshot URL 404s.These are polish-level changes and can be deferred if the current UX is acceptable.
📜 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 (26)
app/adapters/detailed-analysis.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs(3 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss(3 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.scss(2 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.ts(3 hunks)app/components/file-details/vulnerability-analysis-details/index.hbs(8 hunks)app/components/file-details/vulnerability-analysis-details/index.scss(1 hunks)app/models/detailed-analysis.ts(1 hunks)app/styles/_component-variables.scss(1 hunks)app/utils/icons.ts(1 hunks)mirage/factories/detailed-analysis.ts(1 hunks)mirage/models/detailed-analysis.ts(1 hunks)package.json(1 hunks)public/ak-icons.json(1 hunks)scripts/build-icons.mjs(4 hunks)tests/acceptance/breadcrumbs/scan-details-test.js(1 hunks)tests/acceptance/file-details-test.js(2 hunks)tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js(2 hunks)tests/integration/components/file-details/vulnerability-analysis-details/index-test.js(8 hunks)translations/en.json(1 hunks)translations/ja.json(1 hunks)types/ak-icons.d.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- translations/ja.json
- tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js
- app/utils/icons.ts
- tests/acceptance/file-details-test.js
- tests/acceptance/breadcrumbs/scan-details-test.js
- app/components/file-details/vulnerability-analysis-details/index.scss
- package.json
- app/adapters/detailed-analysis.ts
- scripts/build-icons.mjs
- app/components/file-details/vulnerability-analysis-details/findings/index.hbs
- types/ak-icons.d.ts
🧰 Additional context used
🧬 Code graph analysis (4)
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (2)
tests/acceptance/file-details-test.js (2)
analyses(58-60)analyses(246-248)tests/integration/components/file-details/vulnerability-analysis-test.js (4)
analyses(71-77)analyses(144-146)analyses(217-219)i(229-229)
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (2)
app/models/analysis.ts (1)
AnalysisModel(45-291)app/models/detailed-analysis.ts (1)
DetailedFinding(3-7)
mirage/factories/detailed-analysis.ts (2)
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (3)
i(637-637)i(1924-1924)hasScreenshot(104-104)mirage/factories/security/analysis.js (1)
desc(70-70)
app/components/file-details/vulnerability-analysis-details/findings/index.ts (3)
app/models/detailed-analysis.ts (2)
DetailedFinding(3-7)DetailedAnalysisModel(9-12)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
customVulnerableFindings(35-37)app/utils/parse-vulnerable-api-finding.ts (2)
isVulnerableApiFinding(75-90)parseVulnerableApiFinding(97-115)
⏰ 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 (15)
app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs (1)
5-5: LGTM! Typo fix is consistent across the codebase.The CSS class name has been corrected from "analysis-overridded-passed" to "analysis-overridden-passed", fixing the spelling error consistently with other files in this PR.
app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss (1)
50-56: LGTM! CSS selector updated to match corrected class name.The selector has been updated to
.analysis-overridden-passed, matching the typo fix in the corresponding template file.app/components/file-details/vulnerability-analysis-details/index.hbs (1)
66-66: LGTM! Consistent typo fix applied throughout the file.All instances of the CSS class "analysis-overridded-passed" have been corrected to "analysis-overridden-passed".
Also applies to: 102-102, 127-127, 168-168, 197-197, 226-226, 278-278, 308-308
app/components/file-details/vulnerability-analysis-details/findings/index.scss (1)
7-7: LGTM! CSS selector corrected.The class selector has been updated from
.analysis-overridded-passedto.analysis-overridden-passed, consistent with the broader typo fix.app/styles/_component-variables.scss (1)
1187-1194: LGTM! New CSS variables follow established naming conventions.The new screenshot-related CSS custom properties are well-named and consistent with the existing variable structure in this file.
public/ak-icons.json (1)
640-650: LGTM! New icon collection properly structured.The
iconoirCollectionfollows the same structure as existing icon collections and includes the necessary metadata (prefix, icons, lastModified, width, height).mirage/factories/detailed-analysis.ts (2)
5-5: LGTM! ID generation function is correctly implemented.The id generation uses a function that receives the Mirage index and returns a computed ID starting at 101. This pattern correctly generates unique IDs and avoids conflicts with other factories.
7-23: LGTM! Findings generator creates appropriate test data.The findings function generates a well-structured array of test data with randomized content, including conditional screenshot URLs. This aligns with the test patterns seen in the codebase.
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss (2)
7-7: LGTM! CSS modifier corrected.The modifier class has been updated from
.analysis-overridded-passedto.analysis-overridden-passed, consistent with the typo fix throughout the PR.
56-60: LGTM! New CSS classes support screenshot functionality.The new styling for vulnerability finding rows and screenshot UI components (container, image, loading states, buttons) are well-structured and use appropriate CSS custom properties for theming.
Also applies to: 85-112
translations/en.json (1)
1786-1786: Screenshots translation key looks consistentKey name and label align with usages like
{{t 'screenshots'}}in the new UI and follow existing capitalization patterns. No further changes needed.mirage/models/detailed-analysis.ts (1)
1-3: Mirage model stub is appropriateUsing a bare
Model.extend({})is sufficient for the currentdetailed-analysisfixtures and routes; no additional Mirage fields are required right now.tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (1)
632-688: Screenshot assertions nicely mirror the detailed-analysis dataThe new assertions that read
schema.detailedAnalyses.find(analysis.id)and verify titles, descriptions, conditional presence of screenshot buttons, and drawer/image behavior are well-aligned with the seeded data. They effectively couple the UI to the Mirage state without hard-coding indexes or assumptions about which findings have screenshots.app/models/detailed-analysis.ts (1)
3-7: DetailedFinding shape and model registry entry look consistentThe
DetailedFindinginterface (nullabletitle, requireddescription/screenshot) and theModelRegistryaugmentation for'detailed-analysis'are coherent and should integrate cleanly with the rest of the Ember Data setup.Also applies to: 14-18
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
1-38: Component signature, injections, and getters align with newDetailedFindingmodelTyping
customVulnerableFindingsasDetailedFinding[]and exposinganalysis/customVulnerableFindingsvia getters matches the new model and keeps the template API clear. Theintl/storeinjections and tracked UI state properties are also idiomatic for a Glimmer component in this codebase.
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js
Show resolved
Hide resolved
...onents/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs
Show resolved
Hide resolved
...nents/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss
Show resolved
Hide resolved
...ponents/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts
Outdated
Show resolved
Hide resolved
app/components/file-details/vulnerability-analysis-details/findings/index.ts
Outdated
Show resolved
Hide resolved
avzz-19
force-pushed
the
dast-screenshots
branch
from
a1f1aa8 to
625a6c3
Compare
avzz-19
force-pushed
the
dast-screenshots
branch
from
625a6c3 to
867f17d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs (1)
85-109: Simplify screenshot block and keeplocal-classstaticYou still have two
{{#if this.screenshot}}checks and a dynamic utility inlocal-class. You can flatten this and avoid mixing global utilities intolocal-classby:
- Keeping a single outer
{{#if this.screenshot}}.- Making
local-classstatic and moving the conditionaldisplay-noneclass to the normalclassattribute.For example:
- {{#if this.screenshot}} - <AkStack - @alignItems='center' - @justifyContent='center' - class='p-2' - local-class='screenshot-container' - > - {{#if this.screenshot}} - {{#if this.imageLoading}} + {{#if this.screenshot}} + <AkStack + @alignItems='center' + @justifyContent='center' + class='p-2' + local-class='screenshot-container' + > + {{#if this.imageLoading}} <div local-class='loading-container'> <AkStack @alignItems='center' @spacing='2'> <AkLoader @size={{16}} /> <AkTypography>{{t 'loading'}}...</AkTypography> </AkStack> </div> - {{/if}} - - <img - src={{this.screenshot}} - alt={{t 'screenshots'}} - local-class='screenshot-image {{if this.imageLoading "display-none"}}' - {{on 'load' this.onScreenshotLoad}} - data-test-analysisDetails-vulFinding-screenshotImage - /> - {{/if}} + {{/if}} + + <img + src={{this.screenshot}} + alt={{t 'screenshots'}} + local-class='screenshot-image' + class={{if this.imageLoading "display-none"}} + {{on 'load' this.onScreenshotLoad}} + data-test-analysisDetails-vulFinding-screenshotImage + /> </AkStack> {{/if}}This keeps CSS modules usage clean and removes redundant branching.
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
21-61: Trim unused services and state from the componentRight now this class defines several things that are not used anywhere:
@service intland@service store- Tracked
titleanddescription- The
analysisgetterGiven the template only consumes
customVulnerableFindings,screenshotDrawerOpen,screenshot,imageLoading, and the screenshot actions, these extras just add noise and may trip lint rules.Unless you plan to use them imminently, consider removing them to keep the component lean:
- @service declare intl: IntlService; - @service declare store: Store; - - @tracked title: string | null = null; - @tracked description: string = ''; + @service declare intl: IntlService; // or remove if unused everywhere + // @service declare store: Store; // drop if not needed + + // Drop title/description until needed + // @tracked title: string | null = null; + // @tracked description: string = ''; @@ - get analysis() { - return this.args.analysis; - } + // Remove analysis getter if it stays unusedThis will make the intent of the component clearer.
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (1)
98-118: Address the randomized screenshot generation in test setup.The random screenshot generation using
faker.datatype.boolean()(line 104) makes these tests non-deterministic, which was previously flagged by a reviewer. This approach can lead to intermittent test failures and makes debugging difficult.Consider removing the random boolean here and instead using the parameterized
hasScreenshotvalues from individual test cases as shown in lines 577-604.Apply this diff to make the setup deterministic:
- const hasScreenshot = faker.datatype.boolean(); - const detailedFindings = sourceFindings.map((f) => ({ title: f.title ?? null, description: f.description ?? '', - screenshot: hasScreenshot - ? faker.image.urlLoremFlickr({ category: 'technology' }) - : '', + screenshot: '', // Default to no screenshot; individual tests will update as needed }));The individual test cases can then update the detailed-analysis screenshots based on their specific test scenario (as already done in lines 622-631).
🧹 Nitpick comments (4)
app/components/file-details/vulnerability-analysis-details/findings/index.ts (2)
98-104: Remove redundantas DetailedFindingtype assertionGiven
DetailedAnalysisModeldeclaresfindings: DetailedFinding[], thefindingparameter in this loop is alreadyDetailedFinding. The cast is therefore redundant and matches the Sonar hint about an unnecessary assertion.You can simplify to:
- if (!isVulnerableApiFinding(content)) { - customVulnerableFindings.push(finding as DetailedFinding); - } + if (!isVulnerableApiFinding(content)) { + customVulnerableFindings.push(finding); + }This keeps the types accurate without extra noise.
110-123: Consider handling detailed-analysis load failures explicitly
loadDetailedAnalysesdirectly callsthis.store.queryRecord('detailed-analysis', {})after setting the nested namespace. If that request fails (network/API error), the rejection will bubble through the task and may result in unhandled errors without any user feedback.Consider wrapping the query in a
try/catchand using the existing notifications service (or a similar mechanism) to surface a friendly error or at least fail silently with logging, e.g.:- const adapter = this.store.adapterFor('detailed-analysis'); - adapter.setNestedUrlNamespace(String(id)); - - this.detailedAnalysis = (await this.store.queryRecord( - 'detailed-analysis', - {} - )) as DetailedAnalysisModel; + const adapter = this.store.adapterFor('detailed-analysis'); + adapter.setNestedUrlNamespace(String(id)); + + try { + this.detailedAnalysis = (await this.store.queryRecord( + 'detailed-analysis', + {} + )) as DetailedAnalysisModel; + } catch (error) { + // Optionally report via notifications/logging + this.detailedAnalysis = null; + }This would make the component more resilient to backend issues.
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (1)
606-722: Consider extracting helper functions to reduce cognitive complexity.SonarCloud flags this test function with a cognitive complexity of 22, exceeding the allowed limit of 15. While the test logic is comprehensive and correct, the nested conditionals and loops make it harder to maintain.
Consider extracting helper functions for:
- Screenshot button and drawer assertions (lines 681-704)
- Non-screenshot assertions (lines 705-713)
- Finding title/description assertions (lines 671-679)
Example refactor:
// Helper function to test screenshot UI const assertScreenshotUI = async (assert, findingIndex, expectedScreenshotUrl) => { await click( `[data-test-analysisDetails-vulFinding-screenshotBtn="${findingIndex}"]` ); assert .dom('[data-test-analysisDetails-vulFinding-screenshots-drawer]') .exists(); assert .dom('[data-test-analysisDetails-vulFinding-screenshotImage]') .hasAttribute('src', expectedScreenshotUrl); await click( '[data-test-analysisDetails-vulFinding-screenshots-closeBtn]' ); }; // Helper function to assert no screenshot button const assertNoScreenshot = (assert, findingIndex) => { assert .dom(`[data-test-analysisDetails-vulFinding-screenshotBtn="${findingIndex}"`) .doesNotExist(`no screenshot button for finding ${findingIndex} (as expected)`); }; // Then use in the test if (f.screenshot) { assert .dom(`[data-test-analysisDetails-vulFinding-screenshotBtn="${i}"`) .exists(); await assertScreenshotUI(assert, i, f.screenshot); } else { assertNoScreenshot(assert, i); }app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss (1)
85-112: Consider nesting and utility class placement.A previous reviewer suggested applying class nesting for child classes. While
.screenshot-containerand its children appear correctly scoped for a drawer component, there are a few suggestions:
Utility class placement: The
.display-noneutility (lines 96-98) would be better placed in a global utilities file or at the root level of your styles, as it's not specific to screenshots.Consistency check: If
.screenshot-containeris used within a specific component context (like a drawer), consider whether it should be nested under a parent selector for better scoping. For example:.screenshot-drawer { .screenshot-container { width: 500px; align-items: center; justify-content: center; .screenshot-image { max-width: 100%; max-height: 500px; object-fit: contain; } .loading-container { display: flex; padding: 1.5em; align-items: center; justify-content: center; min-height: 530px; box-sizing: border-box; } } }
- Button placement: Consider whether
.screenshot-button(lines 110-112) should be nested under.analysis-sectionsince it references the same CSS variable, or if it belongs in the drawer context.
📜 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 (26)
app/adapters/detailed-analysis.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs(3 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss(3 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.scss(2 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.ts(3 hunks)app/components/file-details/vulnerability-analysis-details/index.hbs(8 hunks)app/components/file-details/vulnerability-analysis-details/index.scss(1 hunks)app/models/detailed-analysis.ts(1 hunks)app/styles/_component-variables.scss(1 hunks)app/utils/icons.ts(1 hunks)mirage/factories/detailed-analysis.ts(1 hunks)mirage/models/detailed-analysis.ts(1 hunks)package.json(1 hunks)public/ak-icons.json(1 hunks)scripts/build-icons.mjs(4 hunks)tests/acceptance/breadcrumbs/scan-details-test.js(1 hunks)tests/acceptance/file-details-test.js(2 hunks)tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js(2 hunks)tests/integration/components/file-details/vulnerability-analysis-details/index-test.js(9 hunks)translations/en.json(1 hunks)translations/ja.json(1 hunks)types/ak-icons.d.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/components/file-details/vulnerability-analysis-details/findings/index.hbs
🚧 Files skipped from review as they are similar to previous changes (17)
- types/ak-icons.d.ts
- app/components/file-details/vulnerability-analysis-details/index.scss
- app/utils/icons.ts
- tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js
- app/adapters/detailed-analysis.ts
- tests/acceptance/file-details-test.js
- public/ak-icons.json
- package.json
- app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss
- tests/acceptance/breadcrumbs/scan-details-test.js
- app/models/detailed-analysis.ts
- app/components/file-details/vulnerability-analysis-details/findings/index.scss
- translations/ja.json
- translations/en.json
- app/styles/_component-variables.scss
- app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs
- app/components/file-details/vulnerability-analysis-details/index.hbs
🧰 Additional context used
🧬 Code graph analysis (4)
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
app/models/detailed-analysis.ts (1)
DetailedFinding(3-7)
mirage/factories/detailed-analysis.ts (2)
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (3)
i(668-668)i(1955-1955)hasScreenshot(104-104)mirage/factories/security/analysis.js (1)
desc(70-70)
scripts/build-icons.mjs (1)
app/utils/icons.ts (1)
IconoirIconsSet(196-196)
app/components/file-details/vulnerability-analysis-details/findings/index.ts (3)
app/models/detailed-analysis.ts (2)
DetailedFinding(3-7)DetailedAnalysisModel(9-12)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
customVulnerableFindings(35-37)app/utils/parse-vulnerable-api-finding.ts (2)
isVulnerableApiFinding(75-90)parseVulnerableApiFinding(97-115)
🪛 GitHub Check: SonarCloud Code Analysis
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js
[failure] 606-606: Refactor this function to reduce its Cognitive Complexity from 22 to the 15 allowed.
app/components/file-details/vulnerability-analysis-details/findings/index.ts
[warning] 102-102: This assertion is unnecessary since it does not change the type of the expression.
⏰ 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: Setup & Build Application
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Cloudflare Pages
🔇 Additional comments (12)
mirage/factories/detailed-analysis.ts (1)
7-23: Factory shape and data generation look goodThe
findingsfactory produces realisticDetailedFinding-like objects (title, description, optional screenshot URL/empty string) and a fixed small set, which is ideal for tests. No issues from a Mirage or typing perspective.mirage/models/detailed-analysis.ts (1)
1-3: Minimal Mirage model is appropriateDefining
export default Model.extend({})is sufficient here; attributes are inferred from the factory/fixtures and keeps the Mirage layer simple.scripts/build-icons.mjs (1)
13-55: Iconoir icon set wiring matches existing patternThe new Iconoir import,
IconoirIconsSetusage, collection construction, and inclusion inoutputare consistent with the other icon sets and should make the newiconoir:screenshoticon available without side effects.app/components/file-details/vulnerability-analysis-details/findings/index.ts (1)
84-108: Detailed-analysis parsing and classification logic looks soundThe updated
parseCustomAndApiVulnerableFindingstask correctly:
- Parses vulnerable API findings from
this.analysis.findingsintovulnerableApiFindings.- Awaits
loadDetailedAnalysesand then walksthis.detailedAnalysis?.findings.- Uses
!isVulnerableApiFinding(content)to classify remaining entries intocustomVulnerableFindings.The flow is clear, avoids Ember Array APIs, and the tracked assignments at the end will trigger the right rerendering.
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (5)
130-132: LGTM!The Mirage route for detailed-analysis follows the established pattern and correctly serves detailed-analysis payloads by analysis ID.
577-604: Excellent test parameterization for screenshot scenarios.The explicit
hasScreenshotflag in the test parameters ensures deterministic test behavior and clearly covers both screenshot and non-screenshot scenarios.
622-631: LGTM!The test correctly updates detailed-analysis findings based on the
hasScreenshotparameter before rendering, ensuring deterministic test behavior.
886-908: LGTM!The detailed-analysis creation for the attachment test follows the established pattern and correctly maps findings without screenshot fields, which is appropriate for this test scenario.
1454-1462: LGTM!The detailed-analysis creation in these regulatory test cases follows a consistent pattern and correctly maps findings without screenshots, which is appropriate for these test scenarios.
Also applies to: 1601-1609, 1882-1890
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss (3)
7-7: LGTM!The typo fix from
analysis-overridded-passedtoanalysis-overridden-passedcorrects the spelling and aligns with test expectations.
56-60: LGTM!The new
.vulnerability-finding-rowstyling provides appropriate spacing and visual separation for finding rows using theme-aware CSS variables.
67-67: LGTM!The change from
max-width: 460pxtowidth: 90%provides more flexible responsive behavior, and the removed styling (background, margin, border-radius) has been appropriately moved to.vulnerability-finding-row.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs (1)
25-60: Tidy up screenshot drawer conditions and class bindingsThe new layout and screenshot drawer behavior look good overall. Two small cleanups would make this easier to maintain:
Remove redundant nested
{{#if this.screenshot}}You already gate the drawer body with
{{#if this.screenshot}}at line 85, so the inner{{#if this.screenshot}}around the loader +<img>is unnecessary:Keep
local-classstatic; toggle visibility viaclassIf
"display-none"is intended as a global utility (or even just a simple non‑scoped helper), it’s clearer not to embed it dynamically inlocal-class. Makinglocal-classstatic and usingclass={{if this.imageLoading "display-none"}}avoids mixing CSS‑modules tokens with global classes and matches the pattern used elsewhere.Together, these changes preserve behavior but simplify the template and class handling.
Also applies to: 64-112
translations/ja.json (1)
1823-1823: Localize the newscreenshotskey to JapaneseThe
"screenshots"value is still in English; this should be translated (for example,"スクリーンショット") to be consistent with the rest of the JA locale.- "screenshots": "Screenshots", + "screenshots": "スクリーンショット",tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (1)
98-118: Replace random screenshot generation with deterministic test scenarios.Using
faker.datatype.boolean()to randomly determine screenshot presence makes tests non-deterministic and harder to debug. This setup affects multiple test cases that run with thisbeforeEachhook.As noted in the past review, create separate, explicit test cases for scenarios with and without screenshots instead of randomizing in the setup phase. This ensures reproducible test results and clearer test intent.
🧹 Nitpick comments (3)
tests/acceptance/file-details-test.js (1)
62-67: Avoidasync+maphere; use a simple loop/forEach
this.server.create('detailed-analysis', …)is synchronous, soanalyses.map(async …)creates and discards promises for no benefit. It’s clearer to dropasync/awaitand avoidmapsince you’re not using its return value.For example:
- analyses.map(async (analysis) => { - await this.server.create('detailed-analysis', { - id: analysis.id, - }); - }); + analyses.forEach((analysis) => { + this.server.create('detailed-analysis', { + id: analysis.id, + }); + });This keeps the Mirage fixtures identical while avoiding unnecessary async noise.
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (1)
577-722: Test logic is comprehensive, but consider addressing complexity and data setup.The test correctly validates screenshot rendering, drawer interactions, and conditional UI elements. The explicit
hasScreenshotparameters in test cases (lines 577-605) are a step toward deterministic testing.However, lines 622-631 mutate the randomly-generated detailed-analysis data to match test expectations, which works around the non-deterministic setup issue flagged earlier. Combined with the SonarCloud cognitive complexity warning (22 vs. allowed 15), consider refactoring:
- Extract helper functions: Move screenshot assertion logic into a dedicated helper (similar to
assertPcidssValue,assertHipaaValuepatterns already used in this file).- Remove data mutation workaround: Once the random generation is replaced with deterministic setup, remove lines 622-631.
Example helper extraction:
this.assertScreenshotRendering = async (assert, findingIndex, screenshot) => { assert .dom(`[data-test-analysisDetails-vulFinding-screenshotBtn="${findingIndex}"]`) .exists(); await click(`[data-test-analysisDetails-vulFinding-screenshotBtn="${findingIndex}"]`); assert .dom('[data-test-analysisDetails-vulFinding-screenshots-drawer]') .exists(); assert .dom('[data-test-analysisDetails-vulFinding-screenshotImage]') .hasAttribute('src', screenshot); await click('[data-test-analysisDetails-vulFinding-screenshots-closeBtn]'); };Based on static analysis hints.
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss (1)
85-112: Consider nesting these styles if they're component-specific.The screenshot styles are functionally correct. However, if
.screenshot-containerand.screenshot-buttonare only used within this component's scope, nesting them under.analysis-sectionwould improve maintainability and prevent potential style conflicts.Based on learnings.
Example refactor:
.analysis-section { // ... existing styles ... .screenshot-container { width: 500px; align-items: center; justify-content: center; .screenshot-image { max-width: 100%; max-height: 500px; object-fit: contain; } .display-none { display: none; } .loading-container { display: flex; padding: 1.5em; align-items: center; justify-content: center; min-height: 530px; box-sizing: border-box; } } .screenshot-button { background-color: var(--file-details-vulnerability-analysis-details-findings-custom-vulnerabilities-background-main); } }Note: If these styles are used in a drawer/modal that renders outside the
.analysis-sectionDOM hierarchy, keep them at the root level as-is.
📜 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 (26)
app/adapters/detailed-analysis.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs(3 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss(3 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.scss(2 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.ts(3 hunks)app/components/file-details/vulnerability-analysis-details/index.hbs(8 hunks)app/components/file-details/vulnerability-analysis-details/index.scss(1 hunks)app/models/detailed-analysis.ts(1 hunks)app/styles/_component-variables.scss(1 hunks)app/utils/icons.ts(1 hunks)mirage/factories/detailed-analysis.ts(1 hunks)mirage/models/detailed-analysis.ts(1 hunks)package.json(1 hunks)public/ak-icons.json(1 hunks)scripts/build-icons.mjs(4 hunks)tests/acceptance/breadcrumbs/scan-details-test.js(1 hunks)tests/acceptance/file-details-test.js(2 hunks)tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js(2 hunks)tests/integration/components/file-details/vulnerability-analysis-details/index-test.js(9 hunks)translations/en.json(1 hunks)translations/ja.json(1 hunks)types/ak-icons.d.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- mirage/models/detailed-analysis.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/acceptance/breadcrumbs/scan-details-test.js
- types/ak-icons.d.ts
- package.json
- app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs
- app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts
- app/adapters/detailed-analysis.ts
- app/utils/icons.ts
- app/components/file-details/vulnerability-analysis-details/findings/index.hbs
- app/models/detailed-analysis.ts
- app/components/file-details/vulnerability-analysis-details/index.scss
- mirage/factories/detailed-analysis.ts
- public/ak-icons.json
- app/components/file-details/vulnerability-analysis-details/findings/index.scss
🧰 Additional context used
🧬 Code graph analysis (3)
scripts/build-icons.mjs (1)
app/utils/icons.ts (1)
IconoirIconsSet(198-198)
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (2)
tests/acceptance/file-details-test.js (2)
analyses(58-60)analyses(246-248)tests/integration/components/file-details/vulnerability-analysis-test.js (4)
analyses(71-77)analyses(144-146)analyses(217-219)i(229-229)
app/components/file-details/vulnerability-analysis-details/findings/index.ts (3)
app/models/detailed-analysis.ts (2)
DetailedFinding(3-7)DetailedAnalysisModel(9-12)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
customVulnerableFindings(35-37)app/utils/parse-vulnerable-api-finding.ts (2)
isVulnerableApiFinding(75-90)parseVulnerableApiFinding(97-115)
🪛 GitHub Check: SonarCloud Code Analysis
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js
[failure] 606-606: Refactor this function to reduce its Cognitive Complexity from 22 to the 15 allowed.
app/components/file-details/vulnerability-analysis-details/findings/index.ts
[warning] 102-102: This assertion is unnecessary since it does not change the type of the expression.
⏰ 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: Setup & Build Application
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Cloudflare Pages
🔇 Additional comments (15)
app/styles/_component-variables.scss (1)
1223-1230: Well-structured variables for screenshots feature.The new screenshot component variables follow established naming conventions, map to valid design tokens, and provide adequate coverage for the drawer and image UI (borders, backgrounds, layout, and disabled states). Placement is logical alongside related file-details sections.
app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss (1)
50-56: Classname typo fix is consistent with template/testsThe
.analysis-overridden-passedselector aligns with the updated template and tests; styling behavior is preserved with corrected spelling.translations/en.json (1)
1823-1823: Newscreenshotstranslation key looks correctKey and value are consistent with the new UI and follow existing naming conventions.
tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js (1)
18-20: Test expectations aligned with corrected classnameThe test description and
hasClass/doesNotHaveClassassertions now match the correctedanalysis-overridden-passedclassname and remain structurally identical otherwise.Also applies to: 73-81
tests/acceptance/file-details-test.js (1)
232-235: Detailed-analysis Mirage route matches new fixture setupThe
/v2/analyses/:id/detailed-analysishandler correctly looks upschema.detailedAnalysesbyidand mirrors how/v2/analyses/:idis stubbed above, so the acceptance test has a realistic backend for the new detailed-analysis page.app/components/file-details/vulnerability-analysis-details/index.hbs (1)
64-69: Overridden-passed styling hook is consistently renamedAll conditional
local-classbindings now use"analysis-overridden-passed"for the passed/overridden state, matching the SCSS and tests without altering control flow.Also applies to: 96-103, 124-128, 166-169, 195-198, 224-227, 276-279, 306-309
scripts/build-icons.mjs (1)
13-14: Iconoir collection wiring is correct; dependency verifiedThe new Iconoir imports,
iconoirCollectionconstruction, and inclusion inoutputfollow the same pattern as the other icon sets. Verification confirms@iconify-json/iconoiris properly declared as a devDependency (v^1.2.10) in package.json, so the script will run without issues.app/components/file-details/vulnerability-analysis-details/findings/index.ts (3)
1-17: LGTM!The new imports for Store, DetailedAnalysisModel, and DetailedFinding are correctly added and properly utilized throughout the component.
26-34: LGTM!The property type updates correctly reflect the new detailed-analysis data model, and the store service is properly declared for data loading.
110-123: LGTM!The detailed-analysis loading task is correctly implemented with proper guard clauses, adapter configuration, and store query logic.
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (3)
130-132: LGTM!The Mirage route for detailed-analysis is correctly implemented and follows the established pattern for other endpoints in this test file.
886-908: LGTM!Correctly creates a corresponding detailed-analysis record for the new analysis, maintaining test data consistency.
1454-1462: LGTM!Consistently creates detailed-analysis records for new analyses across multiple test cases, ensuring proper test data setup for the detailed-analysis integration.
Also applies to: 1601-1609, 1882-1890
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss (2)
7-7: LGTM!Correctly fixes the typo from
analysis-overridded-passedtoanalysis-overridden-passed, using the proper past participle form.
56-67: LGTM!The new
.vulnerability-finding-rowstyling provides consistent visual separation for findings, and the width adjustment accommodates the new screenshot UI elements.
app/components/file-details/vulnerability-analysis-details/findings/index.ts
Show resolved
Hide resolved
avzz-19
force-pushed
the
dast-screenshots
branch
2 times, most recently
from
b5144f6 to
2bf3799
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/components/file-details/vulnerability-analysis-details/findings/index.ts (1)
98-103: Remove redundantas DetailedFindingassertion
detailedAnalysis.findingsis already typed asDetailedFinding[], sofindingin this loop is aDetailedFinding. The cast adds noise and was also flagged by Sonar as unnecessary.You can simplify:
- this.detailedAnalysis?.findings?.forEach((finding) => { + this.detailedAnalysis?.findings?.forEach((finding) => { const content = finding.description; if (!isVulnerableApiFinding(content)) { - customVulnerableFindings.push(finding as DetailedFinding); + customVulnerableFindings.push(finding); } });app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
22-29: Clean up unused state/service if they’re not requiredIn this component,
storeis injected but never used, and the trackedtitle/descriptionfields are not mutated anywhere in the class. If the template is not reading these, they are effectively dead code and can be dropped; if the template does read them, consider setting them inopenScreenshotModalso the drawer has consistent metadata.This keeps the public surface minimal and avoids confusion about unused dependencies.
🧹 Nitpick comments (4)
tests/acceptance/file-details-test.js (1)
79-83: Avoid usingmap+async/awaitfor side‑effects when seeding Mirage
analyses.map(async ...)is only used for creating Mirage records and theawaitis unnecessary here, which makes the code look more asynchronous than it is.You can simplify to a plain
forEachwithoutasync/await:- analyses.map(async (analysis) => { - await this.server.create('detailed-analysis', { - id: analysis.id, - }); - }); + analyses.forEach((analysis) => { + this.server.create('detailed-analysis', { id: analysis.id }); + });app/components/file-details/vulnerability-analysis-details/findings/index.ts (1)
84-108: Detailed‑analysis loading + classification flow looks solid (consider graceful failure)The task sequence—load detailed‑analysis, classify
analysis.findingsinto API vs non‑API, then fillcustomVulnerableFindingsfromdetailedAnalysis.findings—is consistent and uses plain array operations, which avoids the earlierpushObjectsissue.You may want to consider a small hardening so a failed detailed‑analysis request doesn’t break the component (e.g., wrap
await this.loadDetailedAnalyses.perform(this.analysis.id);in a try/catch and fall back to an emptydetailedAnalysis).Also applies to: 110-123
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (2)
12-12: Drop unusedosimport from tests
import { type } from 'os';is unused in this file and pulls in a Node core module into a browser‑oriented Ember test bundle. It also triggers Sonar warnings.You can safely remove it:
-import { type } from 'os';This makes the test file cleaner and avoids any bundler quirks around Node built‑ins.
589-737: Consider extracting screenshot assertions to reduce test complexityThe
test.each('it renders analysis details with vulnerabilities', ...)block now handles many branches (risk combinations, findings presence, and screenshot/no‑screenshot flows), which pushes cognitive complexity quite high.You could pull the per‑finding screenshot expectations (loop over
detailedAnalysis.findings, open/close drawer, assert image src / absence of button) into a small helper function to make the main test case easier to follow while preserving coverage.
📜 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 (26)
app/adapters/detailed-analysis.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs(3 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss(3 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.scss(2 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.ts(3 hunks)app/components/file-details/vulnerability-analysis-details/index.hbs(8 hunks)app/components/file-details/vulnerability-analysis-details/index.scss(1 hunks)app/models/detailed-analysis.ts(1 hunks)app/styles/_component-variables.scss(1 hunks)app/utils/icons.ts(1 hunks)mirage/factories/detailed-analysis.ts(1 hunks)mirage/models/detailed-analysis.ts(1 hunks)package.json(1 hunks)public/ak-icons.json(1 hunks)scripts/build-icons.mjs(4 hunks)tests/acceptance/breadcrumbs/scan-details-test.js(1 hunks)tests/acceptance/file-details-test.js(2 hunks)tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js(2 hunks)tests/integration/components/file-details/vulnerability-analysis-details/index-test.js(10 hunks)translations/en.json(1 hunks)translations/ja.json(1 hunks)types/ak-icons.d.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- tests/acceptance/breadcrumbs/scan-details-test.js
- tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js
- translations/en.json
- app/components/file-details/vulnerability-analysis-details/index.scss
- app/components/file-details/vulnerability-analysis-details/findings/index.scss
- mirage/models/detailed-analysis.ts
- types/ak-icons.d.ts
- app/utils/icons.ts
- public/ak-icons.json
- mirage/factories/detailed-analysis.ts
- translations/ja.json
- app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss
- app/adapters/detailed-analysis.ts
- app/models/detailed-analysis.ts
- scripts/build-icons.mjs
- app/components/file-details/vulnerability-analysis-details/index.hbs
- app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs
- app/styles/_component-variables.scss
🧰 Additional context used
🧬 Code graph analysis (3)
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (6)
tests/integration/components/file-details/vulnerability-analysis-test.js (6)
analyses(78-91)analyses(152-154)analyses(225-227)analyses(295-306)analysis(79-83)i(237-237)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
analysis(31-33)app/components/file-details/vulnerability-analysis-details/findings/index.ts (1)
analysis(48-50)app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts (1)
analysis(43-45)app/components/security/analysis-list/table/action/index.ts (1)
analysis(45-47)app/components/file-details/vulnerability-analysis-details/index.ts (1)
analysis(18-20)
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
app/models/detailed-analysis.ts (1)
DetailedFinding(3-7)
app/components/file-details/vulnerability-analysis-details/findings/index.ts (2)
app/models/detailed-analysis.ts (2)
DetailedFinding(3-7)DetailedAnalysisModel(9-12)app/utils/parse-vulnerable-api-finding.ts (2)
isVulnerableApiFinding(75-90)parseVulnerableApiFinding(97-115)
🪛 GitHub Check: SonarCloud Code Analysis
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js
[warning] 12-12: Remove this unused import of 'type'.
[warning] 12-12: Prefer node:os over os.
[failure] 621-621: Refactor this function to reduce its Cognitive Complexity from 22 to the 15 allowed.
app/components/file-details/vulnerability-analysis-details/findings/index.ts
[warning] 102-102: This assertion is unnecessary since it does not change the type of the expression.
⏰ 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 (5)
package.json (1)
63-63: Dependency addition looks good.The
@iconify-json/iconoirpackage is correctly added with a reasonable semver constraint, properly alphabetized among other iconify-json packages, and aligns with the PR's goal of introducing screenshot icon support.app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs (1)
5-5: Consistent overridden‑passed class nameThe updated class name matches the corrected
.analysis-overridden-passedselector used elsewhere; no behavior change, just consistency.app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss (1)
7-27: Vulnerability row + screenshot styles look coherentThe new
.vulnerability-finding-rowlayout and screenshot‑related styles (.screenshot-container, nested image/loading helpers) align with the updated markup and the correctedanalysis-overridden-passedclass; nothing stands out as risky.Also applies to: 56-60, 66-82, 85-112
app/components/file-details/vulnerability-analysis-details/findings/index.hbs (1)
3-6: Template now uses the canonical overridden‑passed classUsing
"analysis-overridden-passed"here keeps template, styles, and tests in sync for the overridden‑as‑passed state.tests/acceptance/file-details-test.js (1)
240-242: Detailed‑analysis route wiring for acceptance test looks correctThe
/v2/analyses/:id/detailed-analysisMirage route returns the matchingdetailed-analysisrecord by id and should satisfy the component’s data needs during navigation.
avzz-19
force-pushed
the
dast-screenshots
branch
from
2bf3799 to
f812e60
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs (1)
25-60: Simplify redundant{{#if this.screenshot}}nesting in the drawerWithin the drawer, the inner
{{#if this.screenshot}}wrapped around the loading/image block is redundant because the entire<AkStack>is already gated by an outer{{#if this.screenshot}}. You can safely drop the inner{{#if}}to reduce nesting while keeping behavior identical.Also applies to: 64-112
app/adapters/detailed-analysis.ts (1)
3-11: Ensure_buildURL()is actually used by the adapter chainThe URL logic and
setNestedUrlNamespacenow sensibly usethis.namespace_v2to build/v2/analyses/:id/detailed-analysis, and the adapter is correctly registered as'detailed-analysis'. However, per earlier reviews, this_buildURL()override will only take effect ifCommonDRFAdapter/CommondrfNestedAdapterdelegatebuildURL()to_buildURL(). If that delegation is still missing, Ember Data will keep using the parentbuildURL()implementation and ignore this custom URL.Please confirm that:
CommonDRFAdapter(or another base) overridesbuildURL()and internally callsthis._buildURL(...), or- You instead override
buildURL()directly onDetailedAnalysisAdapterto returnthis.buildURLFromBase(\${this.namespace}/detailed-analysis`)`.Otherwise, detailed-analysis requests may hit an incorrect endpoint.
You can verify the delegation with a small scan of the adapters:
#!/bin/bash set -euo pipefail echo "=== Locate CommonDRFAdapter and related files ===" fd -e ts -e js 'commondrf' app/adapters echo echo "=== Inspect buildURL implementations in commondrf* adapters ===" fd -e ts -e js 'commondrf' app/adapters | while read -r file; do echo "== $file ==" rg -n "class CommonDRFAdapter|class CommondrfNestedAdapter|buildURL\\s*\\(" "$file" -n -C3 || true echo doneAlso applies to: 13-17
app/components/file-details/vulnerability-analysis-details/findings/index.ts (1)
98-104: Remove unnecessary type assertion.The type assertion
finding as DetailedFindingon line 102 is redundant sincefindingis already typed asDetailedFindingfrom thethis.detailedAnalysis?.findingsiteration.Apply this diff:
this.detailedAnalysis?.findings?.forEach((finding) => { const content = finding.description; if (!isVulnerableApiFinding(content)) { - customVulnerableFindings.push(finding as DetailedFinding); + customVulnerableFindings.push(finding); } });Based on static analysis hints.
🧹 Nitpick comments (4)
app/components/file-details/vulnerability-analysis-details/findings/index.ts (1)
110-123: Consider adding error handling for the API call.The
loadDetailedAnalysestask fetches data from the API but lacks error handling. If the API call fails, the task will throw, potentially causing an unhandled rejection. Consider wrapping in try-catch or handling via.catch()on the task invocation site if graceful degradation is desired.loadDetailedAnalyses = task(async (id?: string) => { // If no id is provided, do nothing if (!id) { return; } + try { const adapter = this.store.adapterFor('detailed-analysis'); adapter.setNestedUrlNamespace(String(id)); this.detailedAnalysis = (await this.store.queryRecord( 'detailed-analysis', {} )) as DetailedAnalysisModel; + } catch (error) { + // Log or handle gracefully - component can still render without detailed analysis + console.warn('Failed to load detailed analysis:', error); + } });tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (3)
105-131: Non-deterministichasScreenshotinbeforeEachreduces test reliability.Using
faker.datatype.boolean()(line 112) forhasScreenshotintroduces randomness in test setup. While thetest.eachblock later overwrites this value (lines 636-645), having random data inbeforeEachcan cause confusion and may lead to flaky tests if any assertion inadvertently depends on the initial random state.Consider using a deterministic value in
beforeEachsince individual tests explicitly set the screenshot state:analyses.forEach((analysis) => { const sourceFindings = analysis.findings ?? analysis.get?.('findings') ?? (analysis.attrs && analysis.attrs.findings) ?? []; - const hasScreenshot = faker.datatype.boolean(); + const hasScreenshot = false; // Tests explicitly set this per scenario const detailedFindings = sourceFindings.map((f) => ({ title: f.title ?? null, description: f.description ?? '', screenshot: hasScreenshot ? faker.image.urlLoremFlickr({ category: 'technology' }) : '', }));
620-735: Consider extracting assertion helpers to reduce cognitive complexity.SonarCloud flags this test function for exceeding the cognitive complexity threshold (22 vs 15 allowed). While thorough testing is valuable, extracting reusable assertion helpers can improve readability and maintainability.
Consider extracting assertion blocks into helper methods:
// Define helper methods within the module scope const assertFindingTitle = (assert, index, title) => { if (title) { assert.dom(`[data-test-analysisDetails-vulFindingTitle="${index}"]`).hasText(title); } }; const assertFindingDescription = (assert, index, description) => { assert.dom(`[data-test-analysisDetails-vulFindingDescription="${index}"]`).hasText(description); }; const assertScreenshotPresent = async (assert, index, screenshot) => { assert.dom(`[data-test-analysisDetails-vulFinding-screenshotBtn="${index}"]`).exists(); await click(`[data-test-analysisDetails-vulFinding-screenshotBtn="${index}"]`); assert.dom('[data-test-analysisDetails-vulFinding-screenshots-drawer]').exists(); assert.dom('[data-test-analysisDetails-vulFinding-screenshotImage]').hasAttribute('src', screenshot); await click('[data-test-analysisDetails-vulFinding-screenshots-closeBtn]'); }; const assertScreenshotAbsent = (assert, index) => { assert.dom(`[data-test-analysisDetails-vulFinding-screenshotBtn="${index}"]`).doesNotExist(); };Then use these in the test body to reduce nesting and improve clarity.
914-922: Consider extracting detailed-analysis creation to a reusable helper.The pattern for creating detailed-analysis records is repeated across multiple tests (lines 914-922, 1465-1473, 1612-1620, 1893-1901). A helper function would reduce duplication and improve maintainability.
// Add to module scope or hooks const createDetailedAnalysisForAnalysis = (server, analysis) => { const sourceFindings = analysis.findings ?? []; const detailedFindings = sourceFindings.map((f) => ({ title: f.title ?? null, description: f.description ?? '', screenshot: '', })); return server.create('detailed-analysis', { id: analysis.id, findings: detailedFindings, }); }; // Usage in tests: createDetailedAnalysisForAnalysis(this.server, newAnalysis);
📜 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 (26)
app/adapters/detailed-analysis.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs(3 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss(3 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.scss(2 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.ts(3 hunks)app/components/file-details/vulnerability-analysis-details/index.hbs(8 hunks)app/components/file-details/vulnerability-analysis-details/index.scss(1 hunks)app/models/detailed-analysis.ts(1 hunks)app/styles/_component-variables.scss(1 hunks)app/utils/icons.ts(1 hunks)mirage/factories/detailed-analysis.ts(1 hunks)mirage/models/detailed-analysis.ts(1 hunks)package.json(1 hunks)public/ak-icons.json(1 hunks)scripts/build-icons.mjs(4 hunks)tests/acceptance/breadcrumbs/scan-details-test.js(1 hunks)tests/acceptance/file-details-test.js(2 hunks)tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js(2 hunks)tests/integration/components/file-details/vulnerability-analysis-details/index-test.js(9 hunks)translations/en.json(1 hunks)translations/ja.json(1 hunks)types/ak-icons.d.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- translations/ja.json
- app/components/file-details/vulnerability-analysis-details/findings/index.hbs
- tests/acceptance/file-details-test.js
- scripts/build-icons.mjs
- tests/acceptance/breadcrumbs/scan-details-test.js
- public/ak-icons.json
- app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs
- app/components/file-details/vulnerability-analysis-details/index.scss
- app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts
- translations/en.json
- app/components/file-details/vulnerability-analysis-details/findings/index.scss
- app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss
- mirage/factories/detailed-analysis.ts
- app/styles/_component-variables.scss
- app/models/detailed-analysis.ts
🧰 Additional context used
🧬 Code graph analysis (4)
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (5)
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
analysis(31-33)app/components/file-details/vulnerability-analysis-details/findings/index.ts (1)
analysis(48-50)app/components/file-details/vulnerability-analysis-details/edit-analysis-button/index.ts (1)
analysis(43-45)app/components/security/analysis-list/table/action/index.ts (1)
analysis(45-47)app/components/file-details/vulnerability-analysis-details/index.ts (1)
analysis(18-20)
types/ak-icons.d.ts (1)
app/utils/icons.ts (1)
IconoirIconsSet(198-198)
app/adapters/detailed-analysis.ts (1)
app/adapters/commondrf-nested.ts (1)
CommondrfNestedAdapter(5-30)
app/components/file-details/vulnerability-analysis-details/findings/index.ts (3)
app/models/detailed-analysis.ts (2)
DetailedFinding(3-7)DetailedAnalysisModel(9-12)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
customVulnerableFindings(35-37)app/utils/parse-vulnerable-api-finding.ts (2)
isVulnerableApiFinding(75-90)parseVulnerableApiFinding(97-115)
🪛 GitHub Check: SonarCloud Code Analysis
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js
[failure] 620-620: Refactor this function to reduce its Cognitive Complexity from 22 to the 15 allowed.
app/components/file-details/vulnerability-analysis-details/findings/index.ts
[warning] 102-102: This assertion is unnecessary since it does not change the type of the expression.
⏰ 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 (11)
mirage/models/detailed-analysis.ts (1)
1-3: Mirage detailed-analysis model definition is fine as a stubA plain
Model.extend({})is sufficient for Mirage here and matches the usage pattern; no issues from this change.tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js (1)
18-20: Test expectations correctly updated for renamed CSS classThe test name and
.hasClass/.doesNotHaveClassassertions now match the correctedanalysis-overridden-passedclass; this keeps tests in sync with the template.Also applies to: 73-81
app/components/file-details/vulnerability-analysis-details/index.hbs (1)
64-67: Consistent fix ofanalysis-overridden-passedclass nameAll conditionals now use the corrected
analysis-overridden-passedclass, keeping markup consistent across description, CVSS, compliant/non-compliant, regulatory, business implications, and attachments sections.Also applies to: 99-103, 124-128, 166-169, 195-198, 224-227, 276-279, 306-309
app/utils/icons.ts (1)
197-198: IconoirIconsSet export matches existing icon set patternDefining
IconoirIconsSet = ['screenshot'] as constis consistent with the other icon sets and is ready for type integration and build-icons usage.types/ak-icons.d.ts (1)
11-12: Iconoir icon types are correctly wired into AkIconsSet and variantsImporting
IconoirIconsSet, definingIconoirIcon, extendingAkIconsSet, and adding theiconoir:${IconoirIcon}branch inAkIconVariantTypeall follow the existing icon-type pattern and should type-check cleanly with TS 5.x.If you see any TS friction, double-check that all call sites using
iconoir:screenshotare typed asAkIconVariantTypeand thatscripts/build-icons.mjsis updated to include the Iconoir collection.Also applies to: 23-24, 35-36, 47-48, 61-62
package.json (1)
63-63: Iconoir icon JSON devDependency aligns with existing icon pipelineAdding
@iconify-json/iconoir@^1.2.10is a solid choice. The package is actively maintained (published a month ago), uses MIT licensing, and follows the same pattern as other@iconify-json/*dependencies. The version constraint allows for compatible updates and has minimal dependencies (only@iconify/types).app/components/file-details/vulnerability-analysis-details/findings/index.ts (2)
1-11: LGTM: Import and type updates are well-organized.The new imports for
DetailedAnalysisModel,DetailedFinding, and theStoretype are correctly structured and align with the feature requirements for loading detailed analysis data.
26-34: LGTM: Property and service declarations are correct.The tracked properties correctly use
DetailedFinding[]andDetailedAnalysisModel | nulltypes, and the store service injection follows Ember conventions.tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (3)
142-144: LGTM: Mirage route for detailed-analysis is correctly configured.The route handler properly returns the detailed analysis record by ID, aligning with the adapter's nested URL namespace pattern.
591-628: Good parameterization for screenshot scenarios.The test cases now explicitly include
hasScreenshotflags, enabling deterministic testing of both screenshot-present and screenshot-absent scenarios across different risk states.
636-645: LGTM: Dynamic test data setup for screenshots.The test correctly fetches and updates the detailed analysis record to match the parameterized
hasScreenshotvalue, ensuring deterministic behavior per test case.
app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss
Show resolved
Hide resolved
avzz-19
force-pushed
the
dast-screenshots
branch
from
f812e60 to
93e4c5d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/components/file-details/vulnerability-analysis-details/findings/index.ts (1)
98-104: Remove unnecessary type assertion (regression from past review).The type assertion
as DetailedFindingon line 102 is unnecessary since TypeScript can infer thatfindingis already of typeDetailedFindingfromthis.detailedAnalysis?.findings. This was flagged in a previous review (commit f812e60 claimed to address it) but the assertion is still present.Apply this diff:
this.detailedAnalysis?.findings?.forEach((finding) => { const content = finding.description; if (!isVulnerableApiFinding(content)) { - customVulnerableFindings.push(finding as DetailedFinding); + customVulnerableFindings.push(finding); } });Based on static analysis hints and past review comments.
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (1)
105-131: Use deterministic test data instead of random screenshot generation.The test setup uses
faker.datatype.boolean()to randomly determine screenshot presence (line 112), making tests non-deterministic. This conflicts with the test cases at lines 591-645 that explicitly parameterizehasScreenshot.Consider refactoring to make screenshot presence deterministic based on analysis index or test parameters:
- const hasScreenshot = faker.datatype.boolean(); + // Make deterministic based on analysis index + const hasScreenshot = i % 2 === 0; // or based on test parametersOr better yet, move the detailed-analysis creation into individual tests where screenshot presence is explicitly controlled by test parameters.
Based on past review comments.
🧹 Nitpick comments (3)
translations/en.json (1)
1823-1823: Consider alphabetical ordering for translation keys.The new
"screenshots"translation key is placed before"serviceNow"(line 1824), but alphabetically it should appear after"serviceNow". While this is minor and won't affect functionality, maintaining alphabetical order in translation files improves maintainability and consistency.Consider moving the key to a position after
"serviceNow"and its nested block to maintain alphabetical ordering.Note: The AI-generated summary mentions this key appearing in two locations, but only one occurrence is marked in the annotated code provided.
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
25-26: Remove unused tracked properties.The
titleanddescriptionproperties are declared but never set or used in the component. Consider removing them unless they're needed for future functionality.Apply this diff to clean up:
- @tracked title: string | null = null; - @tracked description: string = ''; @tracked screenshotDrawerOpen = false;tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (1)
636-645: Test data mutation couples to non-deterministic setup.The test mutates detailed-analysis screenshot data based on the
hasScreenshotparameter, but this relies on the initial random setup inbeforeEach. This creates a coupling between the test setup and individual test cases.Consider creating the detailed-analysis records with the correct screenshot state directly in individual test cases, rather than mutating them here. This would eliminate the dependency on the random setup and make tests more self-contained.
📜 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 (26)
app/adapters/detailed-analysis.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs(3 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss(3 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.scss(2 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.ts(3 hunks)app/components/file-details/vulnerability-analysis-details/index.hbs(8 hunks)app/components/file-details/vulnerability-analysis-details/index.scss(1 hunks)app/models/detailed-analysis.ts(1 hunks)app/styles/_component-variables.scss(1 hunks)app/utils/icons.ts(1 hunks)mirage/factories/detailed-analysis.ts(1 hunks)mirage/models/detailed-analysis.ts(1 hunks)package.json(1 hunks)public/ak-icons.json(1 hunks)scripts/build-icons.mjs(4 hunks)tests/acceptance/breadcrumbs/scan-details-test.js(1 hunks)tests/acceptance/file-details-test.js(2 hunks)tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js(2 hunks)tests/integration/components/file-details/vulnerability-analysis-details/index-test.js(9 hunks)translations/en.json(1 hunks)translations/ja.json(1 hunks)types/ak-icons.d.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/components/file-details/vulnerability-analysis-details/findings/index.hbs
🚧 Files skipped from review as they are similar to previous changes (15)
- package.json
- tests/acceptance/file-details-test.js
- app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs
- translations/ja.json
- app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs
- app/adapters/detailed-analysis.ts
- app/components/file-details/vulnerability-analysis-details/findings/index.scss
- app/models/detailed-analysis.ts
- app/utils/icons.ts
- types/ak-icons.d.ts
- tests/acceptance/breadcrumbs/scan-details-test.js
- app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss
- mirage/factories/detailed-analysis.ts
- mirage/models/detailed-analysis.ts
- app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss
🧰 Additional context used
🧬 Code graph analysis (3)
scripts/build-icons.mjs (1)
app/utils/icons.ts (1)
IconoirIconsSet(198-198)
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
app/models/detailed-analysis.ts (1)
DetailedFinding(3-7)
app/components/file-details/vulnerability-analysis-details/findings/index.ts (3)
app/models/detailed-analysis.ts (2)
DetailedFinding(3-7)DetailedAnalysisModel(9-12)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
customVulnerableFindings(35-37)app/utils/parse-vulnerable-api-finding.ts (2)
isVulnerableApiFinding(75-90)parseVulnerableApiFinding(97-115)
🪛 GitHub Check: SonarCloud Code Analysis
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js
[failure] 620-620: Refactor this function to reduce its Cognitive Complexity from 22 to the 15 allowed.
app/components/file-details/vulnerability-analysis-details/findings/index.ts
[warning] 102-102: This assertion is unnecessary since it does not change the type of the expression.
⏰ 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 (13)
app/styles/_component-variables.scss (2)
1223-1230: Variable mappings are correct and follow established patterns.Assuming no duplicates exist, the new screenshot variables properly map to existing design tokens and maintain naming consistency with the codebase. The seven variables provide appropriate styling hooks for screenshot UI components (border-radius, background colors, border colors, and disabled state).
1223-1230: [No action required]These screenshot variables are declared only once in the file (lines 1224-1230) with no duplicates. The variable declarations are properly formed and follow consistent naming conventions.
public/ak-icons.json (1)
645-655: NewiconoirCollectionblock is structurally consistent with other icon setsThe added
iconoirCollection(prefix"iconoir","screenshot"icon, metadata, and the comma aftersolarCollection) is consistent with the existing collections and looks like a valid output ofscripts/build-icons.mjs. No issues from a JSON or schema perspective; just keep regenerating this file via the script rather than manual edits going forward to avoid drift.scripts/build-icons.mjs (1)
13-55: Iconoir icon set integration is properly wiredThe new
IconoirIcons/IconoirIconsSetfollows the same pattern as existing icon collections and is correctly integrated. The@iconify-json/iconoirdependency is declared inpackage.json(pinned to^1.2.10), andIconoirIconsSetis properly exported inapp/utils/icons.tswith['screenshot']as expected.app/components/file-details/vulnerability-analysis-details/index.scss (1)
58-58: LGTM! Typo fix applied correctly.The class name has been corrected from
analysis-overridded-passedtoanalysis-overridden-passed, fixing the spelling of "overridden". This aligns with similar corrections throughout the codebase.app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
39-61: LGTM! Screenshot modal actions are well-implemented.The screenshot modal workflow is cleanly implemented with:
- Proper guard clause to prevent opening with empty screenshots
- Loading state management for better UX
- Clean state reset on close
tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js (1)
19-80: LGTM! Test assertions updated to match the typo fix.The test description and assertions have been correctly updated to expect the class name
analysis-overridden-passedinstead of the misspelledanalysis-overridded-passed.app/components/file-details/vulnerability-analysis-details/index.hbs (1)
66-66: LGTM! Typo fix applied consistently throughout the template.All instances of the class name have been corrected from
analysis-overridded-passedtoanalysis-overridden-passed. The changes are consistent and correct.Also applies to: 102-102, 127-127, 168-168, 197-197, 226-226, 278-278, 308-308
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (3)
142-144: LGTM! Mirage route correctly configured.The route for fetching detailed analysis data is properly set up and follows the expected API pattern.
677-733: LGTM! Screenshot assertions are comprehensive.The test thoroughly verifies:
- Screenshot button presence/absence based on data
- Drawer open/close functionality
- Image source attribute correctness
- Proper conditional rendering
900-922: LGTM! Detailed-analysis creation follows consistent pattern.The test data setup consistently creates detailed-analysis records for each analysis, mapping findings appropriately. The approach is correct for the respective test scenarios.
Also applies to: 1465-1473, 1612-1620, 1893-1901
app/components/file-details/vulnerability-analysis-details/findings/index.ts (2)
1-34: LGTM! Type changes and service injection support detailed-analysis feature.The imports and property type changes correctly support the new detailed-analysis integration:
DetailedFindingtype replacesFindingfor custom vulnerabilities- Store service enables fetching detailed-analysis data
detailedAnalysisproperty holds the loaded model
110-123: LGTM! Detailed-analysis loading logic is sound.The
loadDetailedAnalysestask correctly:
- Guards against missing IDs
- Configures the adapter for nested URL construction
- Fetches the detailed-analysis record via
queryRecord
...ponents/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts
Show resolved
Hide resolved
...onents/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs
Outdated
Show resolved
Hide resolved
avzz-19
force-pushed
the
dast-screenshots
branch
from
93e4c5d to
15d8cbd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
app/models/detailed-analysis.ts (1)
9-11: Duplicate: Add defaultValue to prevent runtime crashes on findings property.The past review comment correctly identified this issue:
@attrwithout options defaultsfindingstoundefinedwhen the server omits the field, but the type signature declares it as non-nullableDetailedFinding[]. This creates a type-runtime mismatch that will cause crashes when code calls.lengthor.forEach()directly.app/adapters/detailed-analysis.ts (1)
4-6: Duplicate: _buildURL is never invoked—override buildURL instead.The past review comments correctly identified that
_buildURL()is not called by Ember Data. The parent class'sbuildURL()method should be overridden to use your custom URL logic. Other adapters in the codebase (e.g.,proxy-setting.ts,partnerclient.ts) follow this pattern.app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
22-23: Duplicate: Remove unused service injections.The past review comment remains valid: both
intlandstoreservices are injected but never used. The template uses Ember's{{t}}helper (notthis.intl), andstoreis not referenced anywhere in the component class or the new screenshot modal actions.Based on past review feedback.
app/components/file-details/vulnerability-analysis-details/findings/index.ts (1)
98-104: Remove redundantas DetailedFindingcast
this.detailedAnalysis?.findingsis already typed asDetailedFinding[], andcustomVulnerableFindingsisDetailedFinding[]as well. The cast:customVulnerableFindings.push(finding as DetailedFinding);does not change the type and just adds noise. You can safely drop the assertion:
customVulnerableFindings.push(finding);to keep the types self-documenting and rely on TS inference.
Please re-run
tscor your usual type-check/lint pipeline after this change to confirm no dependent types rely on the explicit cast.
🧹 Nitpick comments (5)
app/adapters/detailed-analysis.ts (1)
8-10: Consider using namespace_v2 property.As noted in a past review comment, you could use
this.namespace_v2instead of manually constructing${this.namespace_v2}/analyses/${id}, assuming the base adapter provides this property.Based on past review feedback.
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (1)
39-48: Consider adding a comment about screenshot validation.The early return when
!screenshotprevents opening an empty modal, but it might be helpful to clarify why an empty string check is sufficient (i.e., whether the backend can return null/undefined vs empty string).tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (2)
105-131: Make detailed-analysis screenshot fixtures deterministic and hermeticThe Mirage detailed-analysis setup is structurally correct, but two aspects could be improved:
faker.datatype.boolean()here makes screenshot presence random for all analyses. Even if current assertions don’t depend on it, this makes future tests harder to reason about and can introduce subtle flakiness.faker.image.urlLoremFlickr(...)(and below without args) can trigger real network requests when<img>elements render.Consider:
- Using a fixed boolean (e.g. always
false) inbeforeEach, and relying on the parameterized test cases to cover both screenshot/no-screenshot scenarios.- Using static, synthetic URLs (e.g.
'https://example.com/test-image.png'or a data URL) instead ofurlLoremFlickrto keep tests fully offline/hermetic.Please run the relevant test module multiple times (or in watch mode) after making this deterministic change to ensure results remain stable across runs.
Also applies to: 142-144
1465-1473: Consider extracting a helper for repeated detailed-analysis fixture setupThe
detailedFindings = analysis.findings.map(...)+this.server.create('detailed-analysis', { id: analysis.id, findings: detailedFindings })pattern is repeated across several tests.Extracting a small helper (e.g.
createDetailedAnalysisFor(analysis)) in this module or in a Mirage factory/util would:
- Reduce duplication.
- Make it harder to accidentally diverge the fixture shape between tests when the DetailedFinding model evolves.
Optional, but would improve maintainability.
Also applies to: 1612-1621, 1893-1902
app/components/file-details/vulnerability-analysis-details/findings/index.ts (1)
110-123: Be mindful of adapter state when usingsetNestedUrlNamespace
loadDetailedAnalysesmutates the adapter viaadapter.setNestedUrlNamespace(String(id))before callingqueryRecord. This is fine if:
- The detailed-analysis adapter is dedicated to this nested URL pattern, or
setNestedUrlNamespaceis effectively idempotent and only affects the next request.If that method stores state on a shared adapter instance, consider either resetting it after the query or passing the namespace through adapter options instead, to avoid surprising interactions when multiple detailed analyses are loaded in the same run.
It’s worth comparing this pattern with other callers of
setNestedUrlNamespacein the codebase to ensure consistent usage and no shared-state pitfalls.
📜 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 (26)
app/adapters/detailed-analysis.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs(3 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss(3 hunks)app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.hbs(1 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.scss(2 hunks)app/components/file-details/vulnerability-analysis-details/findings/index.ts(3 hunks)app/components/file-details/vulnerability-analysis-details/index.hbs(8 hunks)app/components/file-details/vulnerability-analysis-details/index.scss(1 hunks)app/models/detailed-analysis.ts(1 hunks)app/styles/_component-variables.scss(1 hunks)app/utils/icons.ts(1 hunks)mirage/factories/detailed-analysis.ts(1 hunks)mirage/models/detailed-analysis.ts(1 hunks)package.json(1 hunks)public/ak-icons.json(1 hunks)scripts/build-icons.mjs(4 hunks)tests/acceptance/breadcrumbs/scan-details-test.js(1 hunks)tests/acceptance/file-details-test.js(2 hunks)tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js(2 hunks)tests/integration/components/file-details/vulnerability-analysis-details/index-test.js(9 hunks)translations/en.json(1 hunks)translations/ja.json(1 hunks)types/ak-icons.d.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- app/components/file-details/vulnerability-analysis-details/index.hbs
- app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.hbs
- tests/acceptance/file-details-test.js
- tests/integration/components/file-details/vulnerability-analysis-details/code-box-test.js
- mirage/factories/detailed-analysis.ts
- app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.scss
- app/components/file-details/vulnerability-analysis-details/findings/index.hbs
- app/utils/icons.ts
- translations/ja.json
- package.json
- app/components/file-details/vulnerability-analysis-details/index.scss
- scripts/build-icons.mjs
- types/ak-icons.d.ts
- translations/en.json
- app/components/file-details/vulnerability-analysis-details/findings/code-box/index.scss
🧰 Additional context used
🧬 Code graph analysis (3)
app/adapters/detailed-analysis.ts (1)
app/adapters/commondrf-nested.ts (1)
CommondrfNestedAdapter(5-30)
app/components/file-details/vulnerability-analysis-details/findings/index.ts (2)
app/models/detailed-analysis.ts (2)
DetailedFinding(3-7)DetailedAnalysisModel(9-12)app/utils/parse-vulnerable-api-finding.ts (2)
isVulnerableApiFinding(75-90)parseVulnerableApiFinding(97-115)
app/components/file-details/vulnerability-analysis-details/findings/custom-vulnerabilities/index.ts (2)
app/models/analysis.ts (1)
AnalysisModel(45-291)app/models/detailed-analysis.ts (1)
DetailedFinding(3-7)
⏰ 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 (10)
app/components/file-details/vulnerability-analysis-details/findings/code-box/index.hbs (1)
3-6: Corrected conditional CSS class name is appropriateThe updated
"analysis-overridden-passed"literal keeps the existing conditional behavior intact and fixes the naming typo; nothing else in the template is affected.public/ak-icons.json (1)
645-656: LGTM: Screenshot icon addition is well-formed.The new
iconoirCollectionwith the screenshot icon follows the existing pattern, includes proper SVG body, metadata, and dimensions.app/styles/_component-variables.scss (1)
1223-1231: LGTM: Screenshot CSS variables follow existing patterns.The new CSS custom properties for file-details/screenshots consistently map to existing design tokens and follow the established naming convention used throughout this file.
tests/acceptance/breadcrumbs/scan-details-test.js (1)
68-84: LGTM: Mirage route setup is appropriate for testing.The route handler correctly returns detailed-analysis data with the expected structure (id + findings array), matching the DetailedFinding interface defined in the model.
mirage/models/detailed-analysis.ts (1)
1-3: LGTM: Standard Mirage model definition.This follows the conventional Mirage pattern for defining models.
app/components/file-details/vulnerability-analysis-details/findings/index.scss (2)
7-7: Good fix: Corrects class name typo.Changed
analysis-overridded-passedtoanalysis-overridden-passed, aligning with similar corrections throughout the codebase.
71-71: Good fix: Removes extraneous whitespace.Formatting improvement in the hover selector.
tests/integration/components/file-details/vulnerability-analysis-details/index-test.js (2)
588-607: Screenshot drawer behaviour tests look solidThe parameterized
hasScreenshotcases plus the per-finding loop give good coverage:
- Correctly toggles between
vulnerabilityandevidencestitles based on risk/system-pass logic.- Asserts title/description per finding, and conditionally asserts presence/absence of the screenshot button based on
f.screenshot.- Exercises the full open–assert–close flow for the screenshot drawer and verifies the
<img>srcbinding.No issues from a correctness perspective; this should catch regressions in the screenshot UI nicely.
Also applies to: 622-645, 677-707, 713-727, 731-733
900-924: Ensure new analysis with attachments also has detailed-analysis dataWiring
newAnalysisup with a correspondingdetailed-analysisrecord keeps this test aligned with the component’s new data flow (which always loads detailed analyses). UsingnewAnalysis.findings.map(...)to seed the detailed findings is consistent with the global setup.Looks good as-is.
app/components/file-details/vulnerability-analysis-details/findings/index.ts (1)
26-29: DetailedAnalysis-backed parsing flow looks correctThe refactor to:
- Track
customVulnerableFindingsasDetailedFinding[].- Introduce
detailedAnalysis: DetailedAnalysisModel | null.- First parse
this.analysis.findingsintovulnerableApiFindings, then classifythis.detailedAnalysis.findingsintocustomVulnerableFindingsbased onisVulnerableApiFinding.is coherent and aligns with the new detailed-analysis model. Using spread
push(...parseVulnerableApiFinding(content))on a native array avoids the earlierpushObjectsissue and is idiomatic.No functional issues spotted here.
Also applies to: 34-34, 84-108
|
No description provided.