Open
Conversation
WalkthroughAdds fake-app detection: new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Deploying irenestaging with
|
| Latest commit: |
cb842e3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b1073e54.irenestaging.pages.dev |
| Branch Preview URL: | https://models-fake-app-sk.irenestaging.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/models/sk-fake-app.ts (1)
150-156: Consider extracting magic strings to constants.The string literals
'fake_app'and'brand_abuse'are used for classification comparison. While functional, extracting these to named constants would improve maintainability and reduce typo risks.♻️ Optional refactor to use constants
+const AI_CLASSIFICATION_LABELS = { + FAKE_APP: 'fake_app', + BRAND_ABUSE: 'brand_abuse', +} as const; + get isFakeApp() { - return this.aiClassificationLabel === 'fake_app'; + return this.aiClassificationLabel === AI_CLASSIFICATION_LABELS.FAKE_APP; } get isBrandAbuse() { - return this.aiClassificationLabel === 'brand_abuse'; + return this.aiClassificationLabel === AI_CLASSIFICATION_LABELS.BRAND_ABUSE; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/sk-fake-app.ts` around lines 150 - 156, Extract the magic strings used in the getters into named constants and use those constants in isFakeApp and isBrandAbuse; specifically, create constants like FAKE_APP_LABEL = 'fake_app' and BRAND_ABUSE_LABEL = 'brand_abuse' and replace direct comparisons of this.aiClassificationLabel === 'fake_app' and === 'brand_abuse' with comparisons to those constants so the checks in isFakeApp and isBrandAbuse reference the new constants (and centralize the labels for reuse).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/adapters/sk-fake-app.ts`:
- Around line 67-83: The addToInventory method must validate that skAppId
extracted via skFakeApp.belongsTo('skApp').id() is not null before using it in
_buildNestedURL; add the same null-guard used elsewhere: check skAppId,
log/throw a clear error (or return a rejected Promise) if missing, and only call
this._buildNestedURL(skAppId, skFakeApp.id) and proceed to ajax/normalize/push
when skAppId is present so store.normalize and store.push are never called with
an invalid URL or missing parent id.
- Around line 51-65: The skApp relationship can be unloaded so calling
skFakeApp.belongsTo('skApp').id() may return null; update both ignore() and
addToInventory() to guard that value before using it with _buildNestedURL:
retrieve const skAppId = skFakeApp.belongsTo('skApp').id(); if skAppId is falsy,
throw a clear error (or return/reject) indicating the skApp relationship is not
loaded so you avoid building URLs with "null"; apply the same
null-check/early-exit to addToInventory() where belongsTo('skApp').id() is used.
---
Nitpick comments:
In `@app/models/sk-fake-app.ts`:
- Around line 150-156: Extract the magic strings used in the getters into named
constants and use those constants in isFakeApp and isBrandAbuse; specifically,
create constants like FAKE_APP_LABEL = 'fake_app' and BRAND_ABUSE_LABEL =
'brand_abuse' and replace direct comparisons of this.aiClassificationLabel ===
'fake_app' and === 'brand_abuse' with comparisons to those constants so the
checks in isFakeApp and isBrandAbuse reference the new constants (and centralize
the labels for reuse).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 616dd5cc-ca86-4f4b-8381-238e28b08e81
📒 Files selected for processing (5)
app/adapters/sk-fake-app.tsapp/enums.tsapp/models/sk-app.tsapp/models/sk-fake-app.tsapp/serializers/sk-fake-app.ts
Irene
|
||||||||||||||||||||||||||||||||||
| Project |
Irene
|
| Branch Review |
models-fake-app-sk
|
| Run status |
|
| Run duration | 04m 27s |
| Commit |
|
| Committer | Yibaebi Elliot |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
0
|
Pending
|
1
|
Skipped
|
0
|
Passing
|
30
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/tests/dynamic-scan.spec.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| Dynamic Scan > it tests dynamic scan for an apk file: 132571 |
Test Replay
Screenshots
|
|
Yibaebi
force-pushed
the
models-fake-app-sk
branch
from
f5f4a73 to
2589cae
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mirage/factories/sk-app.ts (1)
87-90:⚠️ Potential issue | 🟠 MajorPin the monitoring/fake-app state in
withApprovedStatus.
app/models/sk-app.tsnow derivesfakeAppDetectionIsDisabled,fakeAppDetectionHasResults, andneedsActionfrommonitoring_enabled,store_monitoring_status, andfake_app_detection_status. Leaving those values random here makes a supposedly neutral approved fixture unstable.🧭 Suggested fix
withApprovedStatus: trait({ approval_status: ENUMS.SK_APPROVAL_STATUS.APPROVED, app_status: ENUMS.SK_APP_STATUS.ACTIVE, + monitoring_enabled: false, + store_monitoring_status: ENUMS.SK_APP_MONITORING_STATUS.DISABLED, + fake_app_detection_status: ENUMS.SK_FAKE_APP_DETECTION_STATUS.DISABLED, + has_store_monitoring_data: false, + has_fake_app_detection_data: false, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mirage/factories/sk-app.ts` around lines 87 - 90, The withApprovedStatus trait currently only pins approval_status and app_status but leaves monitoring_enabled, store_monitoring_status, and fake_app_detection_status random, which makes derived properties (fakeAppDetectionIsDisabled, fakeAppDetectionHasResults, needsAction) unstable; update the mirage/factories/sk-app.ts trait withApprovedStatus to explicitly set monitoring_enabled, store_monitoring_status, and fake_app_detection_status to stable values (e.g., monitoring_enabled: false and deterministic store_monitoring_status and fake_app_detection_status enums that represent a neutral/disabled state) so the model-derived flags are consistent for the approved fixture.
🧹 Nitpick comments (1)
mirage/factories/sk-fake-app.ts (1)
9-10: Preferfaker.image.url()for these icon fields.Faker v8 documents
imageUrl()as deprecated in favor ofurl(). This file already uses the newer API forsk_store.icon, so switching these two fields keeps the factory on one API surface. (v8.fakerjs.dev)♻️ Suggested cleanup
- original_app_icon_url: () => faker.image.imageUrl(), - fake_app_icon_url: () => faker.image.imageUrl(), + original_app_icon_url: () => faker.image.url(), + fake_app_icon_url: () => faker.image.url(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mirage/factories/sk-fake-app.ts` around lines 9 - 10, Change the two factory fields original_app_icon_url and fake_app_icon_url to use the newer Faker API by calling faker.image.url() instead of faker.image.imageUrl(); update both occurrences so they match the existing usage for sk_store.icon and keep the factory consistent with Faker v8's non-deprecated API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/adapters/sk-fake-app.ts`:
- Around line 30-33: The code uses truthiness checks that treat 0 as falsy;
update all checks in this adapter to use explicit null/undefined checks instead:
inside _buildNestedURL replace any if (id) with if (id !== undefined && id !==
null) and similarly replace any checks of query.sk_app_id with if
(query.sk_app_id !== undefined && query.sk_app_id !== null); ensure you update
every occurrence (the checks around nested URL construction in _buildNestedURL
and any other methods in sk-fake-app.ts that currently use truthy checks for
sk_app_id or id) so numeric 0 is treated as a valid ID.
In `@app/models/sk-app.ts`:
- Around line 259-261: The model exposes appMonitoringIsInDisabledState but
callers still use appIsInDisabledState; add a compatibility getter named
appIsInDisabledState that returns the same value as
appMonitoringIsInDisabledState (or !this.monitoringEnabled) in the same class so
existing callers (e.g., in app-list/table/monitoring-status) continue to work
while they are migrated, and consider adding a deprecation comment on
appIsInDisabledState.
- Around line 263-281: The getters totalFakeApps, brandAbuseFakeAppPercentage,
and fakeAppsFakeAppPercentage do not guard against missing fakeAppCounts or a
zero denominator; update totalFakeApps to return 0 if this.fakeAppCounts is
falsy, and change brandAbuseFakeAppPercentage and fakeAppsFakeAppPercentage to
first check that this.fakeAppCounts exists and this.totalFakeApps > 0 and
otherwise return 0 (or a safe default) to avoid division-by-zero/NaN; keep the
existing allFakeAppsCountsAreZero logic but ensure it also handles a missing
this.fakeAppCounts consistently.
In `@app/models/sk-fake-app.ts`:
- Around line 149-150: The ignoreReason attribute on the SKFakeApp model should
be nullable: update the declaration of ignoreReason in app/models/sk-fake-app.ts
(symbol: ignoreReason) so its TypeScript type allows null (e.g. string | null)
and ensure the `@attr` declaration reflects that nullable state (so callers can
handle missing values and fixtures can omit the field).
In `@mirage/factories/sk-fake-app.ts`:
- Around line 108-113: The factory currently sets status: () => 0 (PENDING) but
unconditionally populates reviewed_by, reviewed_on, and ignore_reason, creating
impossible fixtures; change reviewed_by, reviewed_on, and ignore_reason to
return null when status() === 0 (or only populate them when status indicates a
reviewed/ignored state) and keep is_ignored and is_added_to_inventory consistent
with the PENDING status (i.e., is_ignored: () => false and
is_added_to_inventory: () => false remain fine for PENDING); update the
reviewed_* and ignore_reason generators to reference the status() value so
default fake-app state is coherent with status, is_ignored, and
is_added_to_inventory.
In `@mirage/models/sk-fake-app.ts`:
- Around line 3-4: The Mirage model export currently names the association
skInventoryApp which changes the serialized relationship key; rename that
association to skApp so it matches the real model's relationship (update the
exported Model.extend property from skInventoryApp to skApp while still using
belongsTo('sk-inventory-app')), ensuring Mirage fixtures hydrate the parent app
without remapping.
---
Outside diff comments:
In `@mirage/factories/sk-app.ts`:
- Around line 87-90: The withApprovedStatus trait currently only pins
approval_status and app_status but leaves monitoring_enabled,
store_monitoring_status, and fake_app_detection_status random, which makes
derived properties (fakeAppDetectionIsDisabled, fakeAppDetectionHasResults,
needsAction) unstable; update the mirage/factories/sk-app.ts trait
withApprovedStatus to explicitly set monitoring_enabled,
store_monitoring_status, and fake_app_detection_status to stable values (e.g.,
monitoring_enabled: false and deterministic store_monitoring_status and
fake_app_detection_status enums that represent a neutral/disabled state) so the
model-derived flags are consistent for the approved fixture.
---
Nitpick comments:
In `@mirage/factories/sk-fake-app.ts`:
- Around line 9-10: Change the two factory fields original_app_icon_url and
fake_app_icon_url to use the newer Faker API by calling faker.image.url()
instead of faker.image.imageUrl(); update both occurrences so they match the
existing usage for sk_store.icon and keep the factory consistent with Faker v8's
non-deprecated API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b19804c0-1862-426c-97f9-d23c7bc04804
📒 Files selected for processing (13)
app/adapters/sk-fake-app-inventory.tsapp/adapters/sk-fake-app.tsapp/enums.tsapp/models/organization.tsapp/models/sk-app.tsapp/models/sk-fake-app-inventory.tsapp/models/sk-fake-app.tsapp/models/sk-inventory-app.tsapp/serializers/sk-fake-app-inventory.tsapp/serializers/sk-fake-app.tsmirage/factories/sk-app.tsmirage/factories/sk-fake-app.tsmirage/models/sk-fake-app.ts
💤 Files with no reviewable changes (1)
- app/models/sk-inventory-app.ts
✅ Files skipped from review due to trivial changes (1)
- app/models/organization.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/serializers/sk-fake-app.ts
Yibaebi
force-pushed
the
models-fake-app-sk
branch
from
2589cae to
4725e52
Compare
Yibaebi
force-pushed
the
models-fake-app-sk
branch
from
4725e52 to
615ed05
Compare
Yibaebi
force-pushed
the
models-fake-app-sk
branch
from
615ed05 to
18a831c
Compare
Yibaebi
force-pushed
the
models-fake-app-sk
branch
from
18a831c to
736bcc2
Compare
Yibaebi
force-pushed
the
models-fake-app-sk
branch
from
736bcc2 to
cb842e3
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/models/sk-app.ts (2)
263-273:⚠️ Potential issue | 🟠 MajorGuard against
nullfakeAppCountsand division by zero.
totalFakeApps,brandAbuseFakeAppPercentage, andfakeAppsFakeAppPercentagewill throw or returnNaNiffakeAppCountsis undefined/null or iftotalFakeAppsequals 0.Proposed fix
get totalFakeApps() { - return this.fakeAppCounts.brand_abuse + this.fakeAppCounts.fake_app; + return ( + (this.fakeAppCounts?.brand_abuse ?? 0) + + (this.fakeAppCounts?.fake_app ?? 0) + ); } get brandAbuseFakeAppPercentage() { - return (this.fakeAppCounts.brand_abuse / this.totalFakeApps) * 100; + return this.totalFakeApps === 0 + ? 0 + : ((this.fakeAppCounts?.brand_abuse ?? 0) / this.totalFakeApps) * 100; } get fakeAppsFakeAppPercentage() { - return (this.fakeAppCounts.fake_app / this.totalFakeApps) * 100; + return this.totalFakeApps === 0 + ? 0 + : ((this.fakeAppCounts?.fake_app ?? 0) / this.totalFakeApps) * 100; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/sk-app.ts` around lines 263 - 273, Guard against null/undefined fakeAppCounts and division by zero in the getters: update totalFakeApps, brandAbuseFakeAppPercentage, and fakeAppsFakeAppPercentage to first handle this.fakeAppCounts being falsy (use optional chaining or a default object) and ensure totalFakeApps returns 0 if counts are absent; in the percentage getters, if totalFakeApps is 0 return 0 (or a defined fallback) instead of performing the division. Refer to the getters totalFakeApps, brandAbuseFakeAppPercentage, fakeAppsFakeAppPercentage and the fakeAppCounts property when making these checks.
259-261:⚠️ Potential issue | 🟠 MajorBreaking change:
appIsInDisabledStategetter is removed but still referenced by components.Based on the relevant code snippets,
app/components/storeknox/inventory/app-list/table/monitoring-status/index.tsreferencesthis.app?.appIsInDisabledStateat multiple locations (lines 36, 46, 68, 90), andapp/routes/authenticated/storeknox/inventory-details/unscanned-version.tsreferences it at line 20. Without a compatibility getter, disabled apps will not render correctly.Proposed compatibility fix
get appMonitoringIsInDisabledState() { return !this.monitoringEnabled; } + + /** `@deprecated` Use appMonitoringIsInDisabledState instead */ + get appIsInDisabledState() { + return this.appMonitoringIsInDisabledState; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/sk-app.ts` around lines 259 - 261, The code removed the legacy getter appIsInDisabledState that other components still call; restore compatibility by adding a new getter named appIsInDisabledState on the same class that simply returns the same value as the existing appMonitoringIsInDisabledState (or !this.monitoringEnabled) so callers like the monitoring-status component and unscanned-version route continue to work without changes.app/adapters/sk-fake-app.ts (1)
69-83:⚠️ Potential issue | 🟡 MinorAdd null guard for
skAppIdbefore building URLs.
belongsTo('skApp').id()returnsnullif the relationship hasn't been loaded, which would produce invalid URLs like/sk_app/null/sk_fake_app/.... This applies to bothignore()andignoreAndAddToInventory()methods.Proposed fix
async ignore( skFakeApp: SkFakeAppModel, ignoreReason: string ): Promise<SkFakeAppModel> { const skAppId = skFakeApp.belongsTo('skApp').id(); + + if (!skAppId) { + throw new Error('Cannot ignore: skApp relationship is not loaded'); + } + - const url = this._buildNestedURL(skAppId, skFakeApp.id).concat('/ignore'); + const url = this._buildNestedURL(skAppId, skFakeApp.id).concat('/ignore'); // ... rest of method }Apply the same pattern to
ignoreAndAddToInventory().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/adapters/sk-fake-app.ts` around lines 69 - 83, The ignore() (and similarly ignoreAndAddToInventory()) method currently calls skFakeApp.belongsTo('skApp').id() without guarding for null, which yields invalid URLs if the relationship isn't loaded; update both methods to guard the skAppId returned from belongsTo('skApp').id() before calling _buildNestedURL: if skAppId is null/undefined, either throw a clear error (e.g., "Missing skApp relationship on SkFakeAppModel") or resolve/load the relationship first and obtain its id, then proceed to build the URL and make the AJAX call; reference the belongsTo('skApp').id(), _buildNestedURL, ignore, and ignoreAndAddToInventory symbols when applying the change.
🧹 Nitpick comments (5)
tests/acceptance/storeknox/inventory-details/store-version-tables-test.js (1)
121-121: Four test suites skipped without explanation.This file has the most skipped tests in this PR. Consider adding a comment block at the module level explaining why these tests are temporarily disabled and what needs to happen to re-enable them.
Also applies to: 184-184, 627-627, 784-784
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/acceptance/storeknox/inventory-details/store-version-tables-test.js` at line 121, Multiple test suites are skipped using test.skip.each; add a module-level comment at the top of this test file that explains why these tests are temporarily disabled, what specific condition or bug must be resolved to re-enable them, and an expected timeline or owner for follow-up, then add a brief inline comment next to each test.skip.each occurrence referencing that module-level explanation (and any tracking ticket/issue ID) so readers know where to find the rationale and re-enable criteria.tests/acceptance/storeknox/inventory-details/malware-detected-test.js (1)
88-88: Tests skipped without explanation.Same pattern as other test files. Document the reason and track re-enablement.
Also applies to: 210-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/acceptance/storeknox/inventory-details/malware-detected-test.js` at line 88, The test block using test.skip.each is being skipped without explanation; add a short comment directly above the test.skip.each that states the reason for skipping, the expected condition for re-enablement, and a tracking reference (issue/PR or ticket ID), and leave the skip in place until that issue is resolved; also apply the same comment/issue reference for the other skipped occurrence referenced (the one at the other occurrence). Locate the skipped tests by the symbol test.skip.each and update them to include the explanatory comment and link to the tracking issue so future reviewers know when/how to re-enable them.tests/acceptance/storeknox/inventory/app-list-test.js (1)
569-569: Tests skipped without explanation.Same concern as other test files. Document the reason for skipping and track re-enablement.
Also applies to: 641-641
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/acceptance/storeknox/inventory/app-list-test.js` at line 569, The skipped table-driven test uses test.skip.each but has no justification; update each occurrence (the test.skip.each lines at the noted locations) to include a clear explanation and tracking info by adding a short comment/TODO directly above the skip stating why the test is skipped, who owns the follow-up, and a ticket or PR link (or expected re-enable condition), or convert the skip to test.todo if appropriate; ensure every skipped instance in this file has that comment so reviewers can trace and re-enable later.tests/acceptance/storeknox/inventory-details/brand-abuse-test.js (1)
89-89: Tests skipped without explanation.Same concern as other test files in this PR. Consider documenting why these tests are disabled and tracking their re-enablement.
Also applies to: 211-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/acceptance/storeknox/inventory-details/brand-abuse-test.js` at line 89, Tests are being skipped via test.skip.each with no explanation; add a short comment immediately above each test.skip.each instance explaining why the test is disabled, include a TODO that references a tracking ticket or issue ID (or create one) and an expected re-enable condition, and if possible add a brief note on how to reproduce the failure locally (e.g., environment or flaky conditions); make the change for the occurrences around the test.skip.each at the top and the second instance near line 211 so future reviewers know the rationale and can track re-enablement.tests/acceptance/storeknox/inventory-details/app-details-test.js (1)
492-492: Tests skipped without explanation or tracking.Two parameterized test suites ("it toggles monitoring status" and "test: actions list section") are now skipped. Consider adding a TODO comment or linking to a tracking issue explaining why these tests are disabled and when they should be re-enabled. Skipped tests can easily become forgotten technical debt.
Also applies to: 1504-1504
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/acceptance/storeknox/inventory-details/app-details-test.js` at line 492, Two parameterized suites are being skipped with test.skip.each (the suites titled "it toggles monitoring status" and "test: actions list section"); add a brief TODO comment above each skipped test explaining why it's skipped and reference a tracking ticket/issue ID and expected re-enable conditions (e.g. "TODO: re-enable after FIX-1234 — root cause: ..."). Locate the skipped calls to test.skip.each in the file (search for the literal test.skip.each and the suite names) and insert the TODO comment and issue link directly above them so reviewers and CI can trace the technical debt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/sk-app.ts`:
- Around line 166-171: The template still references the old getter
monitoringPendingOrDisabled but the model now defines
storeMonitoringPendingOrDisabled; either update the template to use
`@skInventoryApp.storeMonitoringPendingOrDisabled`, or add a backward-compatible
alias getter monitoringPendingOrDisabled in the model (e.g., a new getter that
returns this.storeMonitoringPendingOrDisabled and optionally emits a deprecation
warning) so legacy templates continue to work.
---
Duplicate comments:
In `@app/adapters/sk-fake-app.ts`:
- Around line 69-83: The ignore() (and similarly ignoreAndAddToInventory())
method currently calls skFakeApp.belongsTo('skApp').id() without guarding for
null, which yields invalid URLs if the relationship isn't loaded; update both
methods to guard the skAppId returned from belongsTo('skApp').id() before
calling _buildNestedURL: if skAppId is null/undefined, either throw a clear
error (e.g., "Missing skApp relationship on SkFakeAppModel") or resolve/load the
relationship first and obtain its id, then proceed to build the URL and make the
AJAX call; reference the belongsTo('skApp').id(), _buildNestedURL, ignore, and
ignoreAndAddToInventory symbols when applying the change.
In `@app/models/sk-app.ts`:
- Around line 263-273: Guard against null/undefined fakeAppCounts and division
by zero in the getters: update totalFakeApps, brandAbuseFakeAppPercentage, and
fakeAppsFakeAppPercentage to first handle this.fakeAppCounts being falsy (use
optional chaining or a default object) and ensure totalFakeApps returns 0 if
counts are absent; in the percentage getters, if totalFakeApps is 0 return 0 (or
a defined fallback) instead of performing the division. Refer to the getters
totalFakeApps, brandAbuseFakeAppPercentage, fakeAppsFakeAppPercentage and the
fakeAppCounts property when making these checks.
- Around line 259-261: The code removed the legacy getter appIsInDisabledState
that other components still call; restore compatibility by adding a new getter
named appIsInDisabledState on the same class that simply returns the same value
as the existing appMonitoringIsInDisabledState (or !this.monitoringEnabled) so
callers like the monitoring-status component and unscanned-version route
continue to work without changes.
---
Nitpick comments:
In `@tests/acceptance/storeknox/inventory-details/app-details-test.js`:
- Line 492: Two parameterized suites are being skipped with test.skip.each (the
suites titled "it toggles monitoring status" and "test: actions list section");
add a brief TODO comment above each skipped test explaining why it's skipped and
reference a tracking ticket/issue ID and expected re-enable conditions (e.g.
"TODO: re-enable after FIX-1234 — root cause: ..."). Locate the skipped calls to
test.skip.each in the file (search for the literal test.skip.each and the suite
names) and insert the TODO comment and issue link directly above them so
reviewers and CI can trace the technical debt.
In `@tests/acceptance/storeknox/inventory-details/brand-abuse-test.js`:
- Line 89: Tests are being skipped via test.skip.each with no explanation; add a
short comment immediately above each test.skip.each instance explaining why the
test is disabled, include a TODO that references a tracking ticket or issue ID
(or create one) and an expected re-enable condition, and if possible add a brief
note on how to reproduce the failure locally (e.g., environment or flaky
conditions); make the change for the occurrences around the test.skip.each at
the top and the second instance near line 211 so future reviewers know the
rationale and can track re-enablement.
In `@tests/acceptance/storeknox/inventory-details/malware-detected-test.js`:
- Line 88: The test block using test.skip.each is being skipped without
explanation; add a short comment directly above the test.skip.each that states
the reason for skipping, the expected condition for re-enablement, and a
tracking reference (issue/PR or ticket ID), and leave the skip in place until
that issue is resolved; also apply the same comment/issue reference for the
other skipped occurrence referenced (the one at the other occurrence). Locate
the skipped tests by the symbol test.skip.each and update them to include the
explanatory comment and link to the tracking issue so future reviewers know
when/how to re-enable them.
In `@tests/acceptance/storeknox/inventory-details/store-version-tables-test.js`:
- Line 121: Multiple test suites are skipped using test.skip.each; add a
module-level comment at the top of this test file that explains why these tests
are temporarily disabled, what specific condition or bug must be resolved to
re-enable them, and an expected timeline or owner for follow-up, then add a
brief inline comment next to each test.skip.each occurrence referencing that
module-level explanation (and any tracking ticket/issue ID) so readers know
where to find the rationale and re-enable criteria.
In `@tests/acceptance/storeknox/inventory/app-list-test.js`:
- Line 569: The skipped table-driven test uses test.skip.each but has no
justification; update each occurrence (the test.skip.each lines at the noted
locations) to include a clear explanation and tracking info by adding a short
comment/TODO directly above the skip stating why the test is skipped, who owns
the follow-up, and a ticket or PR link (or expected re-enable condition), or
convert the skip to test.todo if appropriate; ensure every skipped instance in
this file has that comment so reviewers can trace and re-enable later.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2a3d2ca5-2ffc-42cf-b222-62026f151cca
📒 Files selected for processing (19)
app/adapters/sk-fake-app-inventory.tsapp/adapters/sk-fake-app.tsapp/enums.tsapp/models/organization.tsapp/models/sk-app.tsapp/models/sk-fake-app-inventory.tsapp/models/sk-fake-app.tsapp/models/sk-inventory-app.tsapp/serializers/sk-fake-app-inventory.tsapp/serializers/sk-fake-app.tsmirage/factories/sk-app.tsmirage/factories/sk-fake-app.tsmirage/models/sk-fake-app.tstests/acceptance/storeknox/inventory-details/app-details-test.jstests/acceptance/storeknox/inventory-details/brand-abuse-test.jstests/acceptance/storeknox/inventory-details/malware-detected-test.jstests/acceptance/storeknox/inventory-details/store-version-tables-test.jstests/acceptance/storeknox/inventory-details/unscanned-versions-test.jstests/acceptance/storeknox/inventory/app-list-test.js
💤 Files with no reviewable changes (1)
- app/models/sk-inventory-app.ts
✅ Files skipped from review due to trivial changes (6)
- app/models/organization.ts
- tests/acceptance/storeknox/inventory-details/unscanned-versions-test.js
- mirage/models/sk-fake-app.ts
- app/models/sk-fake-app-inventory.ts
- app/enums.ts
- app/adapters/sk-fake-app-inventory.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- app/serializers/sk-fake-app-inventory.ts
- app/serializers/sk-fake-app.ts
- mirage/factories/sk-fake-app.ts
- mirage/factories/sk-app.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.