fix: remove redundant non-null assertions#3220
Conversation
Summary by CodeRabbit
Note: No user-facing changes in this release. ✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRemoved redundant non-null assertion operators (!) from two frontend test files and normalized a numeric literal in a map test. Test interactions now pass elements directly to fireEvent calls; maxBoundsViscosity changed from 1.0 to 1. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-11-17T17:30:42.139ZApplied to files:
🔇 Additional comments (2)
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}
|
All checks pass locally. Please review when convenient. Thank you! |
Updated the PR. Removed remaining redundant non-null assertions as requested. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx (1)
286-302: Incomplete fix: Lines 308 and 317 still use redundant non-null assertions.The removal of
!fromfireEvent.click(overlay)at lines 290 and 299 aligns with the PR objective. However, lines 308 and 317 still containfireEvent.keyDown(overlay!, ...)with the redundant!assertion. Since bothfireEvent.click()andfireEvent.keyDown()have identical type requirements for their element parameter, these should also be updated for consistency and to fully resolve the Sonar rule violation.Proposed fix
- fireEvent.keyDown(overlay!, { key: 'Enter' }) + fireEvent.keyDown(overlay, { key: 'Enter' })- fireEvent.keyDown(overlay!, { key: ' ' }) + fireEvent.keyDown(overlay, { key: ' ' })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/ChapterMap.test.tsx
🔇 Additional comments (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx (1)
290-290: Changes look good, but fix is incomplete.The removal of non-null assertions from these
fireEvent.clickcalls is correct and improves code clarity. The tests will still fail appropriately ifoverlayis null, making the assertions unnecessary.Note: See previous comment about lines 308 and 317 that still need the same fix.
Also applies to: 299-299
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx (1)
308-308: Complete the non-null assertion removal for consistency.Lines 308 and 317 still contain non-null assertions (
overlay!) infireEvent.keyDowncalls, while lines 290 and 299 had them removed fromfireEvent.clickcalls. Since all four use the sameoverlayvariable fromclosest('button'), the assertions should be removed consistently.🔎 Suggested fix to complete the refactor
const overlay = getByText('Unlock map').closest('button') - fireEvent.keyDown(overlay!, { key: 'Enter' }) + fireEvent.keyDown(overlay, { key: 'Enter' }) expect(mockMap.scrollWheelZoom.enable).toHaveBeenCalled()And similarly for line 317:
const overlay = getByText('Unlock map').closest('button') - fireEvent.keyDown(overlay!, { key: ' ' }) + fireEvent.keyDown(overlay, { key: ' ' }) expect(mockMap.scrollWheelZoom.enable).toHaveBeenCalled()Also applies to: 317-317
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
frontend/__tests__/unit/components/ChapterMap.test.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/ChapterMap.test.tsx
🔇 Additional comments (2)
frontend/__tests__/unit/components/ChapterMap.test.tsx (2)
144-144: LGTM: Numeric literal normalized to satisfy Sonar rule.The change from
1.0to1is stylistically cleaner and satisfies static analysis requirements without affecting behavior.
290-290: LGTM: Non-null assertions correctly removed.The removal of
!operators on lines 290 and 299 aligns with the PR's objective to eliminate redundant non-null assertions and satisfy Sonar rule TypeScript:S4325.Also applies to: 299-299
|
Thanks a lot for the review, guidance, and approval. |
* fix: remove redundant non-null assertions * fix: remove remaining redundant non-null assertions * chore: replace 1.0 with 1 to satisfy sonar rule * chore: remove unintended package-lock.json from PR --------- Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
2 participants
Proposed change
Resolves #3171
This PR removes redundant non-null assertions in the ProjectsDashboardDropDown test file. The values are already guaranteed to exist, so the assertions were unnecessary. This improves readability and avoids redundant TypeScript assertions.
Checklist