Skip to content

Conversation

@ReneWerner87
Copy link
Member

No description provided.

Copilot AI review requested due to automatic review settings December 20, 2025 16:55
@ReneWerner87 ReneWerner87 requested a review from a team as a code owner December 20, 2025 16:55
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 20, 2025

Walkthrough

This pull request refactors string handling operations across multiple files, replacing index-based detection and slicing with modern Go string utilities (strings.Cut, strings.Contains). All changes maintain existing functionality while improving code clarity and using idiomatic Go patterns.

Changes

Cohort / File(s) Summary
Modern string utility refactoring (strings.Cut)
middleware/basicauth/basicauth.go, middleware/cors/utils.go, req.go
Replaced index-based string splitting with strings.Cut for extracting delimited components (credentials, schemes, query parameters). Improves readability and uses idiomatic Go string handling.
String presence detection (strings.Contains)
client/core.go
Changed port detection from strings.Index() to strings.Contains() for checking colon presence in addresses. Semantically equivalent but more explicit intent.
Inline optimization
helpers.go
Inlined temporary variable assignment by directly comparing result of strings.IndexByte() to -1, reducing intermediate variable assignment. No behavioral change.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Consistent refactoring pattern: All changes follow repetitive, well-defined patterns (index → strings.Cut, index → strings.Contains)
  • No behavioral changes: String operations remain functionally identical, reducing cognitive load
  • Error handling continuity: Changes in basicauth.go preserve existing error paths via strings.Cut's success/failure pattern

Possibly related PRs

  • #3922: Parallel refactoring effort modifying the same files (helpers.go, req.go) with similar string utility improvements and index-to-Cut conversions.

Suggested reviewers

  • gaby
  • sixcolors
  • efectn

Poem

🐰 String cuts, so clean and neat,
Index slicing, now obsolete!
Contains and Cut, the modern way,
Refactored code, hip-hip-hooray!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning No pull request description was provided by the author, failing to meet the repository's required template structure and content guidelines. Add a comprehensive description following the template, including purpose, type of change, and relevant checklist items for this refactoring effort.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main refactoring work across multiple files, replacing strings.Index calls with strings.Cut for better readability.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix_modernizer_hints

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 312d2e6 and c5e5b0c.

📒 Files selected for processing (5)
  • client/core.go (1 hunks)
  • helpers.go (1 hunks)
  • middleware/basicauth/basicauth.go (1 hunks)
  • middleware/cors/utils.go (1 hunks)
  • req.go (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Prefer github.com/gofiber/utils/v2 helpers (for example, utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code

Files:

  • helpers.go
  • middleware/basicauth/basicauth.go
  • req.go
  • client/core.go
  • middleware/cors/utils.go
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
🧬 Code graph analysis (1)
req.go (1)
error.go (1)
  • ErrRangeMalformed (32-32)
⏰ 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). (5)
  • GitHub Check: lint
  • GitHub Check: unit (1.25.x, macos-latest)
  • GitHub Check: unit (1.25.x, windows-latest)
  • GitHub Check: repeated
  • GitHub Check: Compare
🔇 Additional comments (7)
middleware/cors/utils.go (1)

10-12: LGTM! Clean refactor to strings.Cut.

The scheme extraction logic is now clearer and more idiomatic. Using strings.Cut with the ok flag provides better semantics than manual index-based slicing.

middleware/basicauth/basicauth.go (1)

94-101: LGTM! Improved credential parsing.

The refactor to strings.Cut makes the username/password extraction clearer and more robust. The error handling with the ok flag is appropriate.

helpers.go (1)

211-211: LGTM! Clean inline refactor.

Removing the temporary variable i is a minor simplification that maintains the same logic while reducing code.

client/core.go (1)

36-39: LGTM! More semantic port detection.

Using strings.Contains instead of strings.Index makes the intent clearer—we only care whether a colon exists, not its position.

req.go (3)

324-328: LGTM! Improved header parsing.

The refactor correctly handles comma-separated values in the X-Forwarded-Host header, extracting the first value when multiple are present. The logic is clear and handles both cases appropriately.


648-653: LGTM! Consistent header value extraction.

The scheme parsing now uses strings.Cut to handle comma-separated proxy headers consistently with the Host() method. The logic correctly prioritizes the first value when multiple are present.


762-770: LGTM! Robust range header parsing.

The refactor to strings.Cut improves the Range header parsing logic:

  • Line 762 cleanly splits on the first "="
  • Line 763 validates the format by rejecting missing or duplicate "=" characters
  • Lines 766 and 770 use the split values appropriately

The validation logic correctly identifies malformed Range headers while maintaining the same behavior as the previous implementation.


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.

@ReneWerner87 ReneWerner87 added this to v3 Dec 20, 2025
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @ReneWerner87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request focuses on a targeted refactoring effort to enhance the clarity and conciseness of string manipulation operations throughout the codebase. By migrating from strings.Index and manual slicing to the strings.Cut function, the changes aim to improve code readability and reduce potential errors associated with manual index handling, resulting in a more robust and maintainable codebase.

Highlights

  • Refactoring to strings.Cut: Replaced multiple instances of strings.Index and manual string slicing with the more idiomatic and readable strings.Cut function across several files.
  • Improved Readability: The use of strings.Cut simplifies string parsing logic, making the code easier to understand and maintain by directly returning the 'before' and 'after' parts, along with a boolean indicating if the separator was found.
  • Code Simplification: In client/core.go and helpers.go, strings.Index was replaced with strings.Contains or its return value was directly used, removing unnecessary variable assignments when only the presence of a character was needed.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@codecov
Copy link

codecov bot commented Dec 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.62%. Comparing base (312d2e6) to head (c5e5b0c).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3956   +/-   ##
=======================================
  Coverage   91.62%   91.62%           
=======================================
  Files         119      119           
  Lines       10190    10190           
=======================================
  Hits         9337     9337           
  Misses        540      540           
  Partials      313      313           
Flag Coverage Δ
unittests 91.62% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors string manipulation code to use strings.Cut for improved readability, replacing older patterns using strings.Index and strings.IndexByte. Note: The PR title contains a typo ("readallity" should be "readability").

Key changes:

  • Replaces strings.Index with strings.Cut to split strings and extract substrings before/after delimiters
  • Simplifies conditional logic by checking the boolean return value from strings.Cut instead of comparing index values to -1
  • Improves variable naming by using descriptive names like before and after instead of array slicing operations

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
req.go Refactored Host(), Scheme(), and Range() methods to use strings.Cut for parsing comma-separated headers and range specifications
middleware/cors/utils.go Updated matchScheme() to use strings.Cut for extracting and comparing URL schemes
middleware/basicauth/basicauth.go Simplified credential parsing using strings.Cut to split username and password
helpers.go Removed unused variable assignment when checking for asterisk character
client/core.go Replaced strings.Index with strings.Contains for port existence check

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request aims to improve readability by replacing strings.Index with strings.Cut. The changes in middleware/basicauth/basicauth.go, middleware/cors/utils.go, and req.go are excellent and achieve this goal effectively. I have a couple of suggestions for client/core.go and helpers.go to further improve consistency and readability in line with the PR's objective. Overall, this is a good refactoring effort.

sixcolors

This comment was marked as duplicate.

Copy link
Member

@sixcolors sixcolors left a comment

Choose a reason for hiding this comment

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

I believe IndexByte is more performant than Contains when searching for a single byte.

@ReneWerner87
Copy link
Member Author

ReneWerner87 commented Dec 20, 2025

I believe IndexByte is more performant than Contains when searching for a single byte.

Yes, I know that indexByte is faster. That was actually just to fix modernizer for now and is an excerpt from your adjustments in the pull request.

https://patch-diff.githubusercontent.com/raw/gofiber/fiber/pull/3946.diff

The performance acceleration comes from my other pull request
#3923

@ReneWerner87 ReneWerner87 merged commit 6808448 into main Dec 20, 2025
26 of 27 checks passed
@ReneWerner87 ReneWerner87 deleted the fix_modernizer_hints branch December 20, 2025 17:19
@github-project-automation github-project-automation bot moved this to Done in v3 Dec 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants