🧹 chore: Refactor configuration management for favicon and envvar middlewares

[gaby]

Summary

• This pull request focuses on refactoring the configuration management for several middleware components, specifically envvar and favicon. The primary objective is to enhance code organization and maintainability by relocating configuration structures and their default merging logic into separate config.go files. This change promotes a more centralized and consistent approach to defining and applying default settings across different middleware, while also streamlining the main middleware constructor functions.

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Refactors configuration handling in EnvVar and Favicon middleware by moving Config structs to separate config.go files and introducing configDefault helper functions to consolidate default initialization and merge logic.

Changes

Cohort / File(s)	Summary
EnvVar middleware configuration refactoring middleware/envvar/config.go, middleware/envvar/envvar.go	Introduces new config.go with exported Config struct (ExportVars field), ConfigDefault instance, and configDefault helper. Removes Config from envvar.go and updates New to use configDefault(config...) for defaulting.
Favicon middleware configuration refactoring middleware/favicon/config.go, middleware/favicon/favicon.go	Introduces new config.go with exported Config struct (FileSystem, Next, File, URL, CacheControl, Data fields), ConfigDefault instance, and configDefault helper. Removes Config from favicon.go and updates New to use configDefault(config...) for merged defaults.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Consistent refactoring pattern applied identically across two middleware packages
• Configuration logic remains functionally equivalent; changes are primarily structural (file reorganization and helper extraction)
• No complex business logic changes or control flow modifications

Possibly related PRs

• #3513: Modifies EnvVar middleware configuration and defaulting logic; related refactoring of Config struct extraction.

Suggested labels

🧹 Updates, v3

Suggested reviewers

• sixcolors
• efectn
• ReneWerner87

Poem

🐰 Config files scattered, now neatly arranged,
In separate folders, the middleware exchanged!
With helpers and defaults, all tidy and clean,
The clearest refactor the rabbit's e'er seen! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check	❓ Inconclusive	The PR description provides a clear summary and testing details, but lacks required sections from the template such as 'Changes introduced', 'Type of change', and the full checklist items.	Fill out the 'Changes introduced' section with relevant checkboxes, specify the 'Type of change', and complete the pre-submission checklist to ensure all template requirements are met.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Refactor envvar middleware config' accurately summarizes the main change: moving configuration to a dedicated file and refactoring the config handling in the envvar middleware.

✨ 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 refactor-favicon-middleware-to-use-config.go

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[gemini-code-assist]

Summary of Changes

Hello @gaby, I'm Gemini Code Assist¹! 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 refactoring the configuration management for several middleware components, specifically envvar and favicon. The primary objective is to enhance code organization and maintainability by relocating configuration structures and their default merging logic into separate config.go files. This change promotes a more centralized and consistent approach to defining and applying default settings across different middleware, while also streamlining the main middleware constructor functions.

Highlights

• Configuration Centralization: The Config struct and ConfigDefault variables for both envvar and favicon middleware have been extracted from their respective main files and moved into new, dedicated config.go files.
• Default Configuration Helper: A new configDefault helper function has been introduced in each config.go file. This function is responsible for merging user-provided configurations with the predefined default values, ensuring consistent initialization.
• Simplified Middleware Constructors: The New constructor functions for both envvar and favicon middleware have been updated to utilize the new configDefault helper, significantly simplifying their internal logic for handling configuration defaults.

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]

Codecov Report

❌ Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.67%. Comparing base (3a13562) to head (f1fdc9b).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
middleware/envvar/config.go	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3898      +/-   ##
==========================================
- Coverage   91.68%   91.67%   -0.02%     
==========================================
  Files         117      119       +2     
  Lines       10030    10039       +9     
==========================================
+ Hits         9196     9203       +7     
- Misses        530      531       +1     
- Partials      304      305       +1     

Flag	Coverage Δ
unittests	91.67% <90.90%> (-0.02%)	⬇️

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.

[gemini-code-assist]

Code Review

This pull request effectively refactors the configuration handling for the envvar middleware by moving the Config struct, default values, and a new helper function into a separate config.go file. This improves code organization and simplifies the middleware's New function.

I noticed that the same refactoring pattern was applied to the favicon middleware, which is a great improvement for consistency. However, this was not mentioned in the pull request summary. It would be helpful to update the summary to reflect the full scope of the changes.

I have a couple of suggestions to improve the new configuration helpers.

[gemini-code-assist]

Assigning ConfigDefault.ExportVars directly shares the map from the global ConfigDefault variable. While it's currently initialized as an empty map, ConfigDefault is a mutable global variable. If any other part of the code were to modify ConfigDefault.ExportVars, it could lead to unexpected side effects for all middleware instances created with a nil ExportVars map. It's safer to initialize a new empty map to prevent such issues.

Suggested change

	if cfg.ExportVars == nil {
	cfg.ExportVars = ConfigDefault.ExportVars
	}
	if cfg.ExportVars == nil {
	cfg.ExportVars = map[string]string{}
	}

[gemini-code-assist]

This check is redundant. ConfigDefault.File is an empty string, so this if block effectively does cfg.File = "" when cfg.File is already "". The logic in the New function correctly handles an empty File path, so this block can be safely removed.

[Copilot]

Pull request overview

This PR refactors the configuration handling for two middleware packages (favicon and envvar) by extracting config structures and defaults into dedicated config.go files. The changes improve code organization without altering functionality.

Key Changes:

• Introduced config.go files for both favicon and envvar middlewares containing Config structs, ConfigDefault variables, and configDefault helper functions
• Simplified middleware constructors by delegating config merging to the new configDefault helpers
• Maintained backward compatibility and existing behavior

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
middleware/favicon/config.go	New file containing Config struct, ConfigDefault variable, and configDefault helper function for merging user-provided config with defaults
middleware/favicon/favicon.go	Removed Config struct and default merging logic, now delegates to configDefault helper from config.go
middleware/envvar/config.go	New file containing Config struct, ConfigDefault variable (with empty map default), and configDefault helper function
middleware/envvar/envvar.go	Removed Config struct, now delegates config merging to configDefault helper from config.go

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

middleware/favicon/config.go (1)

9-72: Config + configDefault pattern looks solid; one tiny simplification possible

The structure (exported Config, exported ConfigDefault, unexported configDefault) is consistent with other middleware and correctly covers all fields used in favicon.go. Defaults for URL and CacheControl are clearly defined here and applied in configDefault, which keeps New simple.

Minor nit: the File field is explicitly defaulted even though ConfigDefault.File is "", so this block is currently a no-op:

if cfg.File == "" {
	cfg.File = ConfigDefault.File
}

You could drop it until there’s a non‑empty default for File to slightly reduce noise, but it’s harmless either way.

middleware/envvar/config.go (1)

3-26: Config helper is correct; small doc and immutability nits

The config type and configDefault helper correctly ensure ExportVars is always non‑nil, which matches how newEnvVar consumes it and keeps the “no config ⇒ expose nothing” behavior.

Two minor polish suggestions:

• The comment // ExportVars specifies the environment variables that should export reads a bit off; consider “that should be exported” for clarity.
• To keep ConfigDefault conceptually immutable, you might default ExportVars to a fresh empty map instead of reusing ConfigDefault.ExportVars when cfg.ExportVars is nil, e.g.:

if cfg.ExportVars == nil {
	cfg.ExportVars = map[string]string{}
}

Current code is still safe since you only read from ExportVars, so this is just future‑proofing and not required.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 35ac7db and f1fdc9b.

📒 Files selected for processing (4)

• middleware/envvar/config.go (1 hunks)
• middleware/envvar/envvar.go (1 hunks)
• middleware/favicon/config.go (1 hunks)
• middleware/favicon/favicon.go (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2024-10-08T19:06:06.583Z

Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:16-26
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the session middleware `Config` struct, `Store` is backed by `fiber.Storage`; they are different entities serving distinct purposes in session management.

Applied to files:

• middleware/envvar/envvar.go

🧬 Code graph analysis (2)

middleware/envvar/config.go (1)

middleware/favicon/config.go (2)

• Config (10-41)
• ConfigDefault (44-49)

middleware/favicon/config.go (1)

middleware/envvar/config.go (2)

• Config (4-7)
• ConfigDefault (10-12)

⏰ 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). (6)

• GitHub Check: Agent
• GitHub Check: Compare
• GitHub Check: repeated
• GitHub Check: unit (1.25.x, macos-latest)
• GitHub Check: unit (1.25.x, windows-latest)
• GitHub Check: lint

🔇 Additional comments (2)

middleware/favicon/favicon.go (1)

20-25: Centralizing defaults via configDefault keeps behavior consistent

Using cfg := configDefault(config...) here cleanly centralizes favicon defaults (URL, Cache-Control, etc.) while preserving the existing behavior of New, and avoids duplicating defaulting logic between files. No issues spotted.

middleware/envvar/envvar.go (1)

23-35: Using configDefault in New keeps envvar behavior unchanged and clearer

Switching to cfg := configDefault(config...) here aligns envvar with the shared config pattern and ensures cfg.ExportVars is consistently defaulted via ConfigDefault when no config (or a nil map) is supplied. The handler behavior (no exposure when ExportVars is empty) remains intact. Looks good.