Fix/remove redundant non null assertions #3306
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRemoved an unused Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 0
🧹 Nitpick comments (2)
frontend/__tests__/unit/components/ChapterMap.test.tsx (2)
286-302: Add an assertion before usingoverlayand address inconsistency.The
closest('button')method returnsElement | null. Removing the!without adding a guarding assertion (e.g.,expect(overlay).toBeInTheDocument()) means the test will fail with an unclear error if the element is not found.Additionally, lines 308 and 317 still use
overlay!forfireEvent.keyDown, creating inconsistency within the same file. Consider either:
- Adding
expect(overlay).toBeInTheDocument()before each usage (preferred), or- Applying the same pattern consistently across all tests in this
describeblock.Suggested fix
it('removes overlay when clicked', () => { const { getByText, queryByText } = render(<ChapterMap {...defaultProps} />) const overlay = getByText('Unlock map').closest('button') + expect(overlay).toBeInTheDocument() fireEvent.click(overlay) expect(queryByText('Unlock map')).not.toBeInTheDocument() }) it('enables scroll wheel zoom when overlay is clicked', () => { const { getByText } = render(<ChapterMap {...defaultProps} />) const overlay = getByText('Unlock map').closest('button') + expect(overlay).toBeInTheDocument() fireEvent.click(overlay) expect(mockMap.scrollWheelZoom.enable).toHaveBeenCalled() })
304-320: Inconsistency: These tests still use non-null assertions.Lines 308 and 317 retain the
!operator (overlay!) while the changes at lines 290 and 299 removed them. For consistency and to satisfy the Sonar rule across the file, consider applying the same pattern here.Suggested fix
it('handles keyboard interaction with Enter key', () => { const { getByText } = render(<ChapterMap {...defaultProps} />) const overlay = getByText('Unlock map').closest('button') - fireEvent.keyDown(overlay!, { key: 'Enter' }) + expect(overlay).toBeInTheDocument() + fireEvent.keyDown(overlay, { key: 'Enter' }) expect(mockMap.scrollWheelZoom.enable).toHaveBeenCalled() }) it('handles keyboard interaction with Space key', () => { const { getByText } = render(<ChapterMap {...defaultProps} />) const overlay = getByText('Unlock map').closest('button') - fireEvent.keyDown(overlay!, { key: ' ' }) + expect(overlay).toBeInTheDocument() + fireEvent.keyDown(overlay, { key: ' ' }) expect(mockMap.scrollWheelZoom.enable).toHaveBeenCalled() })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/unit/components/Card.test.tsxfrontend/__tests__/unit/components/ChapterMap.test.tsxfrontend/__tests__/unit/components/ProjectsDashboardDropDown.test.tsx
💤 Files with no reviewable changes (1)
- frontend/tests/unit/components/Card.test.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 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.
📚 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
📚 Learning: 2025-11-17T16:47:05.578Z
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.
Applied to files:
frontend/__tests__/unit/components/ChapterMap.test.tsx
🔇 Additional comments (1)
frontend/__tests__/unit/components/ProjectsDashboardDropDown.test.tsx (1)
296-342: LGTM!The removal of non-null assertions is properly guarded by
expect(activeItem).toBeDefined()andexpect(inactiveItem).toBeDefined()assertions before eachfireEventcall. This is the correct pattern—runtime assertions validate the elements exist before interacting with them.
|
Thanks for the assignment. I’ve updated the PR and verified all local checks pass. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/__tests__/unit/components/ProjectsDashboardDropDown.test.tsx (3)
300-307: Consider using a type-narrowing pattern instead of relying onexpect().toBeDefined().While the test has a runtime assertion via
expect(activeItem).toBeDefined(), this doesn't narrow the TypeScript type. Removing the non-null assertion (!) meansfireEvent.click()receives a potentiallyundefinedvalue from TypeScript's perspective.If strict null checks are enabled, this could cause compilation errors. Consider using a pattern that both asserts at runtime and narrows the type:
💡 Optional: Type-safe assertion pattern
const items = screen.getAllByTestId('dropdown-item') const activeItem = items.find((item) => item.textContent === 'Active') - expect(activeItem).toBeDefined() - - fireEvent.click(activeItem) + if (!activeItem) { + throw new Error('Active item not found') + } + fireEvent.click(activeItem)Alternatively, you could use a helper like
expect(activeItem).toBeTruthy()combined with a type guard, or use testing-library's built-in queries that throw when elements aren't found.
314-321: Same type-narrowing concern applies here.The
expect(activeItem).toBeDefined()assertion doesn't narrow TypeScript's type foractiveItem. The same refactor pattern suggested above would improve type safety.
328-341: Same pattern with two elements needs type narrowing.Both
activeItemandinactiveItemare potentiallyundefinedfrom TypeScript's perspective afterArray.find(). Theexpect().toBeDefined()calls provide runtime guarantees but don't narrow the types.💡 Optional: Combined type guard
const items = screen.getAllByTestId('dropdown-item') const activeItem = items.find((item) => item.textContent === 'Active') const inactiveItem = items.find((item) => item.textContent === 'Inactive') - expect(activeItem).toBeDefined() - expect(inactiveItem).toBeDefined() - - fireEvent.click(activeItem) - fireEvent.click(inactiveItem) + if (!activeItem || !inactiveItem) { + throw new Error('Expected dropdown items not found') + } + fireEvent.click(activeItem) + fireEvent.click(inactiveItem)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/ProjectsDashboardDropDown.test.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 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.
* fixed zap baseline scan * fix:removed false positives * lint/format * fix:removed false positives * update:followed recommendation * lint/format * Update code * Update code --------- Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]>
…WASP#3211) * Define explicit top-level permissions for GitHub Actions workflows * ci: set empty workflow-level permissions and rely on job scopes * ci: allow gha cache writes via job-level actions permission * Update code --------- Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]>
* Remove unnecessary zero fractions from number literals * Fix: remove unnecessary zero fractions across the codebase
anukalp2804
force-pushed
the
fix/remove-redundant-non-null-assertions
branch
from
36bca4d to
0125683
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.zapconfig:
- Around line 7-8: Fix the typo in the .zapconfig comment: change "credicard" to
"creditcard" in the PII disclosure comment so the line reads "PII disclosure:
false positive creditcard number." and keep the rest of the IGNORE entry (10062
IGNORE https://nest.owasp.(dev|org)/sitemap.xml) unchanged.
🧹 Nitpick comments (3)
.zapconfig (1)
4-5: Consider making the chunk filename regex more flexible.The regex
[a-f0-9]{16}matches exactly 16 hex characters. If Next.js changes its chunk naming convention (e.g., different length or character set), this rule may stop matching. Consider a more flexible pattern like[a-f0-9]+if the goal is to match any hex-named chunk..github/workflows/run-ci-cd.yaml (2)
624-624: Trailing space incmd_options.There's a trailing space at the end of the string. Harmless, but worth cleaning up.
Proposed fix
- cmd_options: '-a -c .zapconfig -r report.html ' + cmd_options: '-a -c .zapconfig -r report.html'
958-968: Same trailing space issue in production scan.Consistent with the staging scan, there's a trailing space in the
cmd_optionsvalue.Proposed fix
- cmd_options: '-a -c .zapconfig -r report.html ' + cmd_options: '-a -c .zapconfig -r report.html'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
backend/poetry.lockis excluded by!**/*.lockdocs/poetry.lockis excluded by!**/*.lockfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
.github/workflows/check-pr-issue.yaml.github/workflows/label-issues.yaml.github/workflows/label-pull-requests.yaml.github/workflows/run-ci-cd.yaml.github/workflows/run-code-ql.yaml.github/workflows/update-nest-test-images.yaml.zapconfigcspell/custom-dict.txtfrontend/__tests__/unit/components/Card.test.tsxfrontend/__tests__/unit/components/ChapterMap.test.tsxfrontend/__tests__/unit/components/DonutBarChart.test.tsxfrontend/__tests__/unit/components/ProjectsDashboardDropDown.test.tsxfrontend/__tests__/unit/data/mockCommitteeDetailsData.tsfrontend/__tests__/unit/data/mockSnapshotData.tsfrontend/package.jsonfrontend/src/components/ChapterMap.tsx
💤 Files with no reviewable changes (1)
- frontend/tests/unit/components/Card.test.tsx
✅ Files skipped from review due to trivial changes (1)
- frontend/src/components/ChapterMap.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/tests/unit/components/ProjectsDashboardDropDown.test.tsx
- frontend/tests/unit/components/ChapterMap.test.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 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.
📚 Learning: 2025-12-21T19:03:59.068Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1139
File: .github/workflows/setup-backend-environment/action.yaml:16-27
Timestamp: 2025-12-21T19:03:59.068Z
Learning: Composite actions (runs: using: composite) execute as steps within the calling job's context and can access the job context, including job.services.* properties (e.g., job.services.<service_id>.id, job.services.<service_id>.ports). Service containers must be defined at the job level, but a composite action's steps can reference them via the job context.
Applied to files:
.github/workflows/label-pull-requests.yaml.github/workflows/run-ci-cd.yaml.github/workflows/label-issues.yaml.github/workflows/update-nest-test-images.yaml.github/workflows/run-code-ql.yaml.github/workflows/check-pr-issue.yaml
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
Applied to files:
frontend/__tests__/unit/components/DonutBarChart.test.tsx
🔇 Additional comments (13)
frontend/__tests__/unit/data/mockCommitteeDetailsData.ts (1)
4-4: LGTM!Removing the trailing
.0from the integer timestamp is a sensible cleanup that improves consistency without affecting functionality.frontend/__tests__/unit/data/mockSnapshotData.ts (1)
88-88: LGTM!Consistent cleanup of the timestamp format, matching the change in
mockCommitteeDetailsData.ts.frontend/__tests__/unit/components/DonutBarChart.test.tsx (1)
340-340: LGTM!The change correctly removes redundant
.0decimal notation from the expected values. In JavaScript,1000000.0and1000000are equivalent, and the rounding math confirms these expectations are accurate for the given input[999999.999, 1000000.001, 2000000.5]..github/workflows/label-pull-requests.yaml (1)
6-7: Good security hardening with explicit empty default permissions.Setting
permissions: {}at the workflow level and granting specific permissions at the job level follows the principle of least privilege. The job's explicit permissions (contents: read,pull-requests: write) correctly override this default..github/workflows/update-nest-test-images.yaml (2)
8-9: Good security hardening with explicit empty default permissions.Setting
permissions: {}at the workflow level follows the principle of least privilege.
17-19: Correct permission addition for GHA cache access.The
actions: writepermission is required for thetype=ghacache operations used in the Docker build steps (lines 37, 40, 51, 54, 66, 69). This enables the workflow to write to the GitHub Actions cache.cspell/custom-dict.txt (1)
148-148: LGTM!Dictionary entry correctly added in alphabetical order to support the
.zapconfigfile..github/workflows/label-issues.yaml (1)
9-10: Good security hardening with explicit empty default permissions.Setting
permissions: {}at the workflow level and grantingissues: writeat the job level follows the principle of least privilege..github/workflows/check-pr-issue.yaml (1)
8-9: Good security hardening with explicit empty default permissions.Setting
permissions: {}at the workflow level is especially important for workflows usingpull_request_target, which runs with elevated privileges. The job's explicit permissions correctly scope access to only what's needed..github/workflows/run-code-ql.yaml (1)
15-16: LGTM! Good security hardening.Adding an empty default
permissions: {}at the workflow level enforces the principle of least privilege. Job-level permissions (lines 20-22) correctly override this default with the minimum required permissions for CodeQL analysis..github/workflows/run-ci-cd.yaml (2)
22-23: LGTM! Consistent security hardening across workflows.Adding the empty default permissions block aligns with the same pattern applied to other workflow files in this PR.
614-616: LGTM! Checkout step is required for config file access.The checkout step is necessary since the ZAP action needs access to the
.zapconfigfile from the repository.frontend/package.json (1)
44-44: LGTM!Patch version bump for framer-motion (12.24.0 → 12.24.7) is a low-risk update. This change appears tangential to the PR's main objective of removing unused props and non-null assertions—consider keeping dependency bumps in separate PRs for cleaner history, but this is fine to include.
| # PII disclosure: false positive credicard number. | ||
| 10062 IGNORE https://nest.owasp.(dev|org)/sitemap.xml |
There was a problem hiding this comment.
Minor typo: "credicard" → "creditcard".
Proposed fix
-# PII disclosure: false positive credicard number.
+# PII disclosure: false positive credit card number.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # PII disclosure: false positive credicard number. | |
| 10062 IGNORE https://nest.owasp.(dev|org)/sitemap.xml | |
| # PII disclosure: false positive credit card number. | |
| 10062 IGNORE https://nest.owasp.(dev|org)/sitemap.xml |
🤖 Prompt for AI Agents
In @.zapconfig around lines 7 - 8, Fix the typo in the .zapconfig comment:
change "credicard" to "creditcard" in the PII disclosure comment so the line
reads "PII disclosure: false positive creditcard number." and keep the rest of
the IGNORE entry (10062 IGNORE https://nest.owasp.(dev|org)/sitemap.xml)
unchanged.
|
Thanks for the update. I’ll wait for further guidance. |
5 participants
Proposed change
Resolves #3244
This PR removes the unused
closeDelayprop from theMockTooltipPropsinterface. The prop was defined but never used, so removing it improves clarity and resolves Sonar ruletypescript:S6767.Checklist