Skip to content

Conversation

@kart-u
Copy link
Contributor

@kart-u kart-u commented Oct 31, 2025

Proposed change

Resolves #2519

Refactored string#replace() with string#replaceAll() at places where replace was used with global regex ensuring compliance with the corresponding SonarQube rule S7781

Checklist

  • I've read and followed the contributing guidelines.
  • I've run make check-test locally; all checks and tests passed.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025

Summary by CodeRabbit

  • Refactor
    • Standardized internal string processing implementations across multiple components, utilities, and test files for improved consistency, readability, and maintainability.
    • Updated text transformation methods used throughout the application, including URL slug generation, label formatting, CSS class normalization, and content processing.
    • Internal maintenance improvements with no impact on user-facing functionality or observable application behavior.

Walkthrough

Replaced uses of String.prototype.replace() that applied global regexes with String.prototype.replaceAll() across tests, components, utilities, and a page; no public API or behavior-introducing logic changes were made.

Changes

Cohort / File(s) Summary
Tests
frontend/__tests__/unit/components/AnchorTitle.test.tsx, frontend/__tests__/unit/components/MarkdownWrapper.test.tsx, frontend/__tests__/unit/pages/UserDetails.test.tsx
Updated mock/test string transformations to use replaceAll() instead of replace() with global regex patterns (slug mocks, markdown mock, badge test).
UI Components
frontend/src/components/Badges.tsx, frontend/src/components/BreadCrumbs.tsx
Replaced replace() calls with replaceAll() in internal normalization/label formatting logic.
Utilities & Pages
frontend/src/utils/slugify.ts, frontend/src/app/board/[year]/candidates/page.tsx
Switched global-regex replace() usages to replaceAll() for diacritic removal, whitespace/newline normalization, and slug generation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Changes are mechanical and repetitive (API swap from replacereplaceAll) across a small set of files.
  • Pay extra attention to:
    • frontend/src/utils/slugify.ts — central utility used widely; ensure behavior unchanged for edge cases.
    • Tests under frontend/__tests__ — verify mocks behave equivalently and test expectations remain valid.
    • frontend/src/app/board/[year]/candidates/page.tsx — confirm slug and bio normalization still produce expected results.

Possibly related PRs

Suggested reviewers

  • arkid15r

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "refactor-replace-with-replaceAll" directly and clearly describes the primary change in the changeset. The entire PR is focused on refactoring calls to String#replace() with global regex patterns to use String#replaceAll() instead, which is exactly what the title conveys. The title is concise, specific, and provides sufficient clarity for someone scanning the project history to understand the main objective.
Linked Issues Check ✅ Passed The PR comprehensively addresses all requirements from linked issue #2519. The changes replace String#replace() with global regex patterns to String#replaceAll() across seven files: AnchorTitle.test.tsx, MarkdownWrapper.test.tsx, UserDetails.test.tsx, candidates/page.tsx, Badges.tsx, BreadCrumbs.tsx, and slugify.ts. Both test files and implementation files are updated as appropriate, and the author confirmed all existing tests pass via make check-test, meeting the compliance criteria that all replace() calls with global regex are converted and existing tests remain passing.
Out of Scope Changes Check ✅ Passed All changes in the PR are directly in scope with the objectives defined in linked issue #2519. The modifications consistently replace String#replace() with global regex patterns to String#replaceAll(), affecting both test files and implementation files. No unrelated features, bug fixes, or refactorings outside the replace/replaceAll scope are introduced, and the functional behavior remains equivalent to the original implementations.
Description Check ✅ Passed The pull request description is clearly related to the changeset. It explains the context by referencing issue #2519, specifies the SonarQube rule S7781 that prompted the refactoring, and mentions that local checks and tests passed. The description adequately explains why the changes were made and provides traceability to the underlying requirement, making it relevant and informative.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0b1aa15 and 024d7de.

📒 Files selected for processing (1)
  • frontend/src/components/BreadCrumbs.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/src/components/BreadCrumbs.tsx

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (9)
frontend/src/utils/slugify.ts (1)

4-5: Consider removing redundant global flags.

The changes correctly migrate to replaceAll(), but the g flag is now redundant since replaceAll() always replaces all occurrences by default.

Apply this diff to remove the redundant flags:

-    .replaceAll(/[\u0300-\u036F]/g, '') // Remove diacritics
-    .replaceAll(/[^a-zA-Z0-9]+/g, '-') // Replace non-alphanumeric with hyphens
+    .replaceAll(/[\u0300-\u036F]/, '') // Remove diacritics
+    .replaceAll(/[^a-zA-Z0-9]+/, '-') // Replace non-alphanumeric with hyphens
frontend/src/components/BreadCrumbs.tsx (1)

46-46: Consider removing redundant global flag.

The migration to replaceAll() is correct, but the g flag is redundant.

Apply this diff:

-            const label = upperFirst(segment).replaceAll(/-/g, ' ')
+            const label = upperFirst(segment).replaceAll(/-/, ' ')
frontend/src/components/Badges.tsx (1)

19-19: Consider removing redundant global flag.

The refactor to replaceAll() is correct, but the g flag is redundant.

Apply this diff:

-  return cssClass.trim().replaceAll(/_([a-z])/g, (_, letter) => letter.toUpperCase())
+  return cssClass.trim().replaceAll(/_([a-z])/, (_, letter) => letter.toUpperCase())
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)

44-44: Consider removing redundant global flag.

The migration to replaceAll() is correct, but the g flag is redundant.

Apply this diff:

-      data-testid={`badge-${name.toLowerCase().replaceAll(/\s+/g, '-')}`}
+      data-testid={`badge-${name.toLowerCase().replaceAll(/\s+/, '-')}`}
frontend/__tests__/unit/components/MarkdownWrapper.test.tsx (1)

14-15: Consider removing redundant global flags.

The migration to replaceAll() is correct, but the g flags are redundant.

Apply this diff:

      return content
-        .replaceAll(/\*\*(.*?)\*\*/g, '<strong>$1</strong>')
-        .replaceAll(/\[(.*?)\]\((.*?)\)/g, '<a href="$2">$1</a>')
+        .replaceAll(/\*\*(.*?)\*\*/, '<strong>$1</strong>')
+        .replaceAll(/\[(.*?)\]\((.*?)\\)/, '<a href="$2">$1</a>')
frontend/src/app/board/[year]/candidates/page.tsx (2)

192-192: Consider removing redundant global flag.

The migration to replaceAll() is correct, but the g flag is redundant.

Apply this diff:

-      const nameSlug = candidate.memberName.toLowerCase().replaceAll(/\s+/g, '_')
+      const nameSlug = candidate.memberName.toLowerCase().replaceAll(/\s+/, '_')

298-298: Consider removing redundant global flags.

The migration to replaceAll() is correct, but the g flags are redundant.

Apply this diff:

-              {candidate.member.bio.replaceAll(/\n+/g, ' ').replaceAll(/\s+/g, ' ').trim()}
+              {candidate.member.bio.replaceAll(/\n+/, ' ').replaceAll(/\s+/, ' ').trim()}
frontend/__tests__/unit/components/AnchorTitle.test.tsx (2)

14-16: Consider removing redundant global flags.

The migration to replaceAll() is correct, but the g flags are redundant.

Apply this diff:

    str
      .toLowerCase()
-      .replaceAll(/[^a-z0-9]/g, '-')
-      .replaceAll(/-{2,10}/g, '-')
-      .replaceAll(/(^-{1,10}|-{1,10}$)/g, '')
+      .replaceAll(/[^a-z0-9]/, '-')
+      .replaceAll(/-{2,10}/, '-')
+      .replaceAll(/(^-{1,10}|-{1,10}$)/, '')

655-657: Consider removing redundant global flags.

The migration to replaceAll() is correct, but the g flags are redundant (consistent with the mock at lines 14-16).

Apply this diff:

        str
          .toLowerCase()
-          .replaceAll(/[^a-z0-9]/g, '-')
-          .replaceAll(/-{2,10}/g, '-')
-          .replaceAll(/(^-{1,10}|-{1,10}$)/g, '')
+          .replaceAll(/[^a-z0-9]/, '-')
+          .replaceAll(/-{2,10}/, '-')
+          .replaceAll(/(^-{1,10}|-{1,10}$)/, '')
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 78812ac and 0b1aa15.

📒 Files selected for processing (7)
  • frontend/__tests__/unit/components/AnchorTitle.test.tsx (2 hunks)
  • frontend/__tests__/unit/components/MarkdownWrapper.test.tsx (1 hunks)
  • frontend/__tests__/unit/pages/UserDetails.test.tsx (1 hunks)
  • frontend/src/app/board/[year]/candidates/page.tsx (2 hunks)
  • frontend/src/components/Badges.tsx (1 hunks)
  • frontend/src/components/BreadCrumbs.tsx (1 hunks)
  • frontend/src/utils/slugify.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.

Applied to files:

  • frontend/__tests__/unit/pages/UserDetails.test.tsx
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.

Applied to files:

  • frontend/src/components/Badges.tsx

Copy link
Collaborator

@kasya kasya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kart-u thanks for working on this! 👍🏼 Looks good!
p.s. Pushed tiny update to get rid of regex with global flag in one place.

@kasya kasya enabled auto-merge November 1, 2025 01:55
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 1, 2025

@kasya kasya added this pull request to the merge queue Nov 1, 2025
Merged via the queue into OWASP:main with commit dffea35 Nov 1, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor: Replace String#replace() with replaceAll() for global regex patterns

2 participants