feat: expose startup message customization hooks#3824
Conversation
Summary of ChangesHello @efectn, 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 enhances the Fiber framework by providing robust customization options for the application's startup message. It moves beyond a static banner, allowing developers to dynamically inject relevant information, warnings, or errors directly into the console output before and after the server starts listening. This improves the developer experience by making the startup process more informative and adaptable to specific application needs. Highlights
Using Gemini Code AssistThe 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
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 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
|
|
Note Other AI code review bot(s) detectedCodeRabbit 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. WalkthroughRefactors startup/listen flow to centralize startup context in a pointer-based Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App
participant Hooks as Hooks
participant Renderer as StartupRenderer
Note over App: prepareListenData(addr,isTLS,cfg,childPIDs) → *ListenData
App->>Hooks: executeOnPreStartupMessageHooks(*PreStartupMessageData)
alt Pre hook sets PreventDefault
Hooks-->>App: PreventDefault=true
Note right of App: Suppress rendering
else Pre hook customizes header/entries
Hooks-->>App: Modified PreStartupMessageData
end
App->>Renderer: printMessages(*ListenData) (render if not prevented/disabled)
Renderer->>Hooks: executeOnPostStartupMessageHooks(*PostStartupMessageData)
Hooks-->>App: PostStartupMessageData (Disabled / IsChild / Prevented)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3824 +/- ##
==========================================
+ Coverage 91.59% 91.65% +0.05%
==========================================
Files 115 115
Lines 9867 9967 +100
==========================================
+ Hits 9038 9135 +97
- Misses 528 532 +4
+ Partials 301 300 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the startup message system by introducing customizable startup message hooks and expanding listener metadata. The changes improve extensibility by allowing users to customize the startup banner through OnPreStartupMessage and OnPostStartupMessage hooks, while also enriching the ListenData structure with comprehensive runtime metadata.
- Introduced
OnPreStartupMessageandOnPostStartupMessagehooks for startup banner customization - Expanded
ListenDatawith version, handler count, process count, PID, and child PID metadata - Refactored
startupMessage()to useListenDatainstead of individual parameters and process startup entries dynamically
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| hooks.go | Added new hook types and data structures for startup message customization with entry management methods |
| listen.go | Refactored startupMessage() to use ListenData and support customizable startup entries, removed trailing whitespace from ASCII art |
| prefork.go | Updated to use childPIDs slice instead of string concatenation, integrated with new startup message flow |
| app.go | Changed startupProcess() return type from *App to void |
| hooks_test.go | Added comprehensive tests for startup message customization and ListenData metadata verification |
| listen_test.go | Updated test calls to use new startupMessage() signature with prepareListenData() |
| prefork_test.go | Updated test to use new startupMessage() signature |
| docs/whats_new.md | Documented new startup message customization features |
| docs/api/hooks.md | Added documentation for ListenData fields and startup message customization API |
There was a problem hiding this comment.
Code Review
This pull request introduces a great feature for customizing the startup message in Fiber, providing more flexibility for developers. The implementation with OnPreStartupMessage and OnPostStartupMessage hooks and the associated data structures is well-designed. The refactoring in listen.go and prefork.go to support this is clean.
However, there are some critical issues with the documentation. The examples in docs/api/hooks.md and docs/whats_new.md are based on an outdated implementation and will not work with the code in this PR. I've provided suggestions to fix them. I also found some dead code in hooks.go that should be removed.
Once these issues are addressed, this will be a solid contribution to Fiber.
docs/api/hooks.md
Outdated
| - Assign `sm.Header` to override the ASCII art banner. Leave it empty to use the default. | ||
| - Provide `sm.PrimaryInfo` and/or `sm.SecondaryInfo` maps to replace the primary (server URL, handler counts, etc.) and secondary (prefork status, PID, process count) sections. | ||
| - Set `sm.PreventDefault = true` to suppress the built-in banner without affecting other hooks. | ||
| - `PostStartupMessageData` reports whether the banner was skipped via the `Disabled`, `IsChild`, and `Prevented` flags. | ||
|
|
||
| ```go title="Customize the startup message" | ||
| package main | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "os" | ||
|
|
||
| "github.com/gofiber/fiber/v3" | ||
| ) | ||
|
|
||
| func main() { | ||
| app := fiber.New() | ||
|
|
||
| app.Hooks().OnPreStartupMessage(func(sm *fiber.PreStartupMessageData) error { | ||
| sm.Header = "FOOBER " + sm.Version + "\n-------" | ||
| sm.PrimaryInfo = fiber.Map{"Git hash": os.Getenv("GIT_HASH")} | ||
| sm.SecondaryInfo = fiber.Map{"Prefork": sm.Prefork} | ||
| return nil | ||
| }) | ||
|
|
||
| app.Hooks().OnPostStartupMessage(func(sm fiber.PostStartupMessageData) error { | ||
| if !sm.Disabled && !sm.IsChild && !sm.Prevented { | ||
| fmt.Println("startup completed") | ||
| } | ||
| return nil | ||
| }) | ||
|
|
||
| app.Listen(":5000") | ||
| } | ||
| ``` |
There was a problem hiding this comment.
The documentation for startup message customization is outdated. The description mentions sm.PrimaryInfo and sm.SecondaryInfo, and the example code uses them, but these fields do not exist on the PreStartupMessageData struct. The new API uses methods like AddInfo, AddWarning, and AddError. The documentation and example should be updated to reflect the correct API usage, as shown in the pull request description.
- Use `sm.AddInfo()`, `sm.AddWarning()`, and `sm.AddError()` to add, update, or remove startup message entries.
- Assign `sm.Header` to override the ASCII art banner. Leave it empty to use the default.
- Set `sm.PreventDefault = true` to suppress the built-in banner without affecting other hooks.
- `PostStartupMessageData` reports whether the banner was skipped via the `Disabled`, `IsChild`, and `Prevented` flags.
```go title="Customize the startup message"
package main
import (
"fmt"
"github.com/gofiber/fiber/v3"
)
func main() {
app := fiber.New()
app.Hooks().OnPreStartupMessage(func(sm *fiber.PreStartupMessageData) error {
sm.Header = "My Awesome App v" + sm.Version + "\n----------------------"
// You can add custom info, warnings, or errors to the startup message.
sm.AddInfo("custom_info", "Custom Info", "This is a custom message.")
sm.AddWarning("custom_warning", "Custom Warning", "This is a custom warning.")
// Entries can be prioritized. Higher priority entries are shown first.
sm.AddError("custom_error", "Custom Error", "This is a custom error.", 10)
return nil
})
app.Hooks().OnPostStartupMessage(func(sm fiber.PostStartupMessageData) error {
if !sm.Disabled && !sm.IsChild && !sm.Prevented {
fmt.Println("startup completed")
}
return nil
})
app.Listen(":5000")
}
docs/whats_new.md
Outdated
| app.Hooks().OnPreStartupMessage(func(sm *fiber.PreStartupMessageData) error { | ||
| sm.Header = "FOOBER " + sm.Version + "\n-------" | ||
| sm.PrimaryInfo = fiber.Map{"Git hash": os.Getenv("GIT_HASH")} | ||
| sm.SecondaryInfo = fiber.Map{"Process count": sm.ProcessCount} | ||
| // Set sm.PreventDefault = true to suppress the default banner entirely. | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
The example code for the new startup message hooks uses sm.PrimaryInfo and sm.SecondaryInfo, which are from an older implementation and no longer exist on PreStartupMessageData. The new API uses methods like AddInfo. This example should be updated to reflect the correct usage.
| app.Hooks().OnPreStartupMessage(func(sm *fiber.PreStartupMessageData) error { | |
| sm.Header = "FOOBER " + sm.Version + "\n-------" | |
| sm.PrimaryInfo = fiber.Map{"Git hash": os.Getenv("GIT_HASH")} | |
| sm.SecondaryInfo = fiber.Map{"Process count": sm.ProcessCount} | |
| // Set sm.PreventDefault = true to suppress the default banner entirely. | |
| return nil | |
| }) | |
| app.Hooks().OnPreStartupMessage(func(sm *fiber.PreStartupMessageData) error { | |
| sm.Header = "FOOBER " + sm.Version + "\n-------" | |
| sm.AddInfo("git_hash", "Git hash", os.Getenv("GIT_HASH")) | |
| sm.AddInfo("process_count", "Process count", fmt.Sprintf("%d", sm.ProcessCount)) | |
| // Set sm.PreventDefault = true to suppress the default banner entirely. | |
| return nil | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
listen.go (1)
53-56: Docstring nit: field name mismatchComment says “ListenerFunc” but the field is ListenerAddrFunc. Update the comment to match the field name to avoid confusion.
-// ListenerFunc allows accessing and customizing net.Listener. +// ListenerAddrFunc allows accessing and customizing net.Listener address.
🧹 Nitpick comments (4)
hooks_test.go (1)
292-352: Harden assertions and avoid unexported fields in tests
- Prefer public API: use app.Config().ColorScheme instead of app.config.ColorScheme.
- Guard slice access before indexing pre.entries.
Apply:
- require.Equal(t, app.config.ColorScheme, data.ColorScheme) + require.Equal(t, app.Config().ColorScheme, data.ColorScheme) @@ - require.Equal(t, app.config.ColorScheme, data.ColorScheme) + require.Equal(t, app.Config().ColorScheme, data.ColorScheme) @@ - require.Equal(t, "value", pre.entries[0].value) + require.Len(t, pre.entries, 2) + require.Equal(t, "value", pre.entries[0].value)prefork_test.go (1)
86-91: Unify stdout capture with captureOutput for consistencyReuse the existing captureOutput helper to simplify and ensure log output is captured identically across tests.
Example:
out := captureOutput(func() { app.startupProcess() listenData := app.prepareListenData(":0", false, cfg, nil) app.startupMessage(listenData, cfg) }) require.Empty(t, out)listen.go (2)
451-459: Avoid builtin min() for wider Go compatibilityIf the repo targets <1.21, builtin min won’t exist. Use explicit bounds check.
- end := min(i+rowTotalPidCount, totalPIDs) + end := i + rowTotalPidCount + if end > totalPIDs { + end = totalPIDs + }[Suggest running the same go.mod/version script shared above to confirm target Go version.]
111-125: Housekeeping: run format/align/modernizePlease run the repo’s formatting targets to align with project standards:
- make format (gofumpt)
- make betteralign
- make modernize (gopls)
As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app.go(1 hunks)docs/api/hooks.md(1 hunks)docs/whats_new.md(1 hunks)hooks.go(6 hunks)hooks_test.go(2 hunks)listen.go(6 hunks)listen_test.go(5 hunks)prefork.go(3 hunks)prefork_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/api/hooks.mddocs/whats_new.md
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)
Files:
app.gohooks_test.golisten_test.goprefork_test.gohooks.golisten.goprefork.go
🧠 Learnings (1)
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
PR: gofiber/fiber#3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.
Applied to files:
docs/whats_new.md
🧬 Code graph analysis (5)
hooks_test.go (3)
app.go (2)
Config(113-418)Version(37-37)listen.go (1)
ListenConfig(42-125)hooks.go (3)
Hooks(36-52)ListenData(75-90)PreStartupMessageData(93-101)
listen_test.go (3)
app.go (1)
New(515-628)listen.go (1)
ListenConfig(42-125)hooks.go (3)
Hooks(36-52)PreStartupMessageData(93-101)PostStartupMessageData(183-189)
prefork_test.go (1)
app.go (1)
New(515-628)
hooks.go (3)
color.go (1)
Colors(8-53)app.go (2)
Version(37-37)Map(43-43)prefork.go (1)
IsChild(31-33)
listen.go (4)
app.go (2)
App(68-110)Version(37-37)hooks.go (1)
ListenData(75-90)prefork.go (1)
IsChild(31-33)color.go (1)
Colors(8-53)
🪛 GitHub Actions: golangci-lint
hooks.go
[error] 93-93: golangci-lint (go vet) error: fieldalignment: struct with 40 pointer bytes could be 32 (govet)
🪛 GitHub Check: lint
hooks.go
[failure] 205-205:
func mapToEntries is unused (unused)
[failure] 93-93:
fieldalignment: struct with 40 pointer bytes could be 32 (govet)
listen.go
[failure] 475-475:
identical-switch-branches: "switch" with identical branches (lines 476 and 482) (revive)
⏰ 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). (3)
- GitHub Check: Compare
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
🔇 Additional comments (11)
prefork.go (2)
92-121: LGTM: child PIDs propagation and hook orderingCollecting
childPIDsand feeding them intoprepareListenData, then runningOnListenbeforestartupMessageis correct and preserves master/child separation.Also applies to: 136-143
96-104: No issues found—Go version supportsfor rangeover intThe project's
go.modspecifiesgo 1.25.0, which fully supports thefor range intValuesyntax introduced in Go 1.22. The code atprefork.go:96is valid and requires no changes.hooks.go (1)
54-63: LGTM: startup message hook surface and executorsExported levels, extended ListenData, and pre/post executors look coherent and minimal.
Also applies to: 74-91, 401-419
app.go (1)
1328-1336: Verified: All call sites correctly updatedThe signature change is complete and consistent. Zero usages attempt to assign the return value from
startupProcess(). All 10 call sites treat it as a void function, confirming the refactoring is correct.listen_test.go (4)
627-663: Nice coverage of header and custom entriesCovers header override, entry reset, ordering, and post-hook flags. LGTM.
664-685: Disabled startup message + post-hook state — solidAsserts both no output and Disabled=true. LGTM.
686-713: Prevented-by-prehook path — good verificationValidates suppression and PostStartupMessage.Prevented. LGTM.
526-534: ****The repository is pinned to Go 1.25.0, making the
for range <int>syntax fully supported and idiomatic. The pattern used at line 528 (and elsewhere throughout the codebase) is appropriate and requires no changes. The original concern about Go 1.22-only syntax is not applicable.listen.go (3)
327-333: printMessages signature change — good separationDelegating to startupMessage(listenData, cfg) keeps concerns clean; routes printing remains gated. LGTM.
335-369: prepareListenData clones child PIDs and fills metadata correctlyNicely avoids aliasing and captures PID, handler/process counts, TLS/prefork flags. LGTM.
371-466: Startup message flow with pre/post hooks looks solid
- Honors DisableStartupMessage, IsChild, and PreventDefault.
- Always fires post-hook via defer.
One suggestion: compute disabled/isChild/prevented after pre-hook (as you do) and return early; already correct. LGTM.
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 8a9167a | Previous: db2576e | Ratio |
|---|---|---|---|
Benchmark_Utils_SortAcceptedTypes_Sorted |
16.54 ns/op 0 B/op 0 allocs/op |
10.75 ns/op 0 B/op 0 allocs/op |
1.54 |
Benchmark_Utils_SortAcceptedTypes_Sorted - ns/op |
16.54 ns/op |
10.75 ns/op |
1.54 |
This comment was automatically generated by workflow using github-action-benchmark.
|
@efectn can you check the AI hints and golang lint |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
hooks.go (1)
90-98: Fix struct field alignment forPreStartupMessageData.The field alignment issue flagged in the previous review remains unaddressed. Please reorder fields as suggested to reduce padding and satisfy golangci-lint.
Apply this diff:
type PreStartupMessageData struct { *ListenData - Header string - entries []startupMessageEntry + Header string + entries []startupMessageEntry PreventDefault bool }As per coding guidelines (betteralign). Based on the previous review, the optimal order is:
entries,Header,*ListenData,PreventDefault.
🧹 Nitpick comments (1)
hooks.go (1)
100-125: Consider extracting priority-parsing logic (optional).The three methods (
AddInfo,AddWarning,AddError) duplicate the priority extraction pattern. While this is acceptable for such short methods, you could optionally extract a helper to eliminate the duplication.Example:
func (sm *PreStartupMessageData) AddInfo(key, title, value string, priority ...int) { sm.addEntry(key, title, value, parsePriority(priority), StartupMessageLevelInfo) } func (sm *PreStartupMessageData) AddWarning(key, title, value string, priority ...int) { sm.addEntry(key, title, value, parsePriority(priority), StartupMessageLevelWarning) } func (sm *PreStartupMessageData) AddError(key, title, value string, priority ...int) { sm.addEntry(key, title, value, parsePriority(priority), StartupMessageLevelError) } func parsePriority(priority []int) int { if len(priority) > 0 { return priority[0] } return -1 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hooks.go(5 hunks)listen.go(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)
Files:
listen.gohooks.go
🧠 Learnings (2)
📚 Learning: 2025-10-22T13:48:12.485Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-22T13:48:12.485Z
Learning: Applies to **/*.go : Optimize struct field alignment using betteralign (Make target: betteralign)
Applied to files:
hooks.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.
Applied to files:
hooks.go
🧬 Code graph analysis (2)
listen.go (5)
app.go (2)
App(68-110)Version(37-37)hooks.go (1)
ListenData(72-87)log/fiberlog.go (1)
Errorf(49-51)prefork.go (1)
IsChild(31-33)color.go (1)
Colors(8-53)
hooks.go (3)
color.go (1)
Colors(8-53)app.go (1)
Version(37-37)prefork.go (1)
IsChild(31-33)
🔇 Additional comments (12)
listen.go (5)
12-12: LGTM!The
ioimport is properly used byprintStartupEntrieswhich accepts anio.Writerparameter.
327-333: LGTM!The updated signature correctly accepts and propagates
ListenDatatostartupMessage.
336-369: LGTM!The function correctly populates
ListenDatawith startup metadata. ThechildPIDscloning (lines 351-354) safely prevents mutations of the caller's slice.
372-466: LGTM!The refactored startup flow correctly uses the data-driven approach with
ListenData. The deferred post-startup hook (lines 419-424) executes correctly even when the banner is suppressed (line 426-428), ensuring hook consistency.
468-486: LGTM! Previous issue resolved.The identical switch branches issue from the previous review has been properly addressed. The function now sets default label/color before the switch and only handles warning/error cases explicitly, eliminating duplication.
hooks.go (7)
18-21: LGTM!The hook signatures correctly use a pointer for
OnPreStartupMessageHandler(allowing mutation) and a value forOnPostStartupMessageHandler(read-only metadata).
51-69: LGTM!The
StartupMessageLevelenum and internalstartupMessageEntrystruct are well-defined. KeepingstartupMessageEntryunexported is appropriate as it's an implementation detail.
72-87: LGTM!The enriched
ListenDataprovides comprehensive startup metadata for hooks and startup message customization.
127-168: LGTM!The entry manipulation methods are correctly implemented.
ResetEntriesefficiently reuses the underlying array, andaddEntryproperly handles both updates and insertions.
170-200: LGTM!Both helper functions correctly clone
ListenDataandChildPIDs, ensuring hooks cannot mutate the original data. Theappend([]int(nil), ...)idiom is idiomatic and efficient.
210-211: LGTM!The new hook slice initializations are consistent with existing patterns.
262-274: LGTM!The hook registration and execution methods follow the established patterns in the codebase and are correctly implemented with proper mutex protection.
Also applies to: 378-396
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)
listen_test.go (1)
513-544: Prefork startup message expectations likely no longer match actual colored outputThe assertions for the Prefork line:
- Line [543]:
require.Contains(t, startupMessage, fmt.Sprintf("Prefork: \t\t\t%sEnabled%s", colors.Blue, colors.Reset))- Line [624]:
require.Contains(t, startupMessage, fmt.Sprintf("Prefork: \t\t\t%sDisabled%s", colors.Red, colors.Reset))use a zero-valued
Colors{}(so the expected substring is literally"Prefork: \t\t\tEnabled"/"Prefork: \t\t\tDisabled"), whilestartupMessagenow formats this line viaprintStartupEntrieswith nested color codes:
- It prints
Prefork: \tthencolors.Blueand then anentry.valuethat itself starts with\t\tand embeds its own color sequences.As a result, there are color escape sequences interleaved between the tabs and the word
Enabled/Disabled, so the raw substring"Prefork: \t\t\tEnabled"/"Prefork: \t\t\tDisabled"is very unlikely to exist, and these tests will probably fail even though the line is rendered correctly.I’d recommend loosening these checks to assert on more robust tokens instead of exact tab layout, for example:
- Check presence of
"Prefork:"and"Enabled"/"Disabled"separately, or- Strip ANSI codes from
startupMessagebefore matching and then assert on the desired layout.This keeps alignment expectations testable without tightly coupling to the internal coloring/format string.
Also applies to: 599-625
♻️ Duplicate comments (1)
hooks.go (1)
162-179: Fix duplicate entries when reusing a key in addEntry
addEntrycurrently:
- Scans existing
sm.entriesand mutates the first matching element (samekey), but- Still appends a new
startupMessageEntryunconditionally afterward.Calling
AddInfo/AddWarning/AddErrorwith the same key will therefore produce duplicate rows with the last call’s value and level, which is surprising and contradicts the “key” semantics.You can fix this by returning immediately after updating an existing entry so the append only happens for new keys:
func (sm *PreStartupMessageData) addEntry(key, title, value string, priority int, level StartupMessageLevel) { if sm.entries == nil { sm.entries = make([]startupMessageEntry, 0) } for i, entry := range sm.entries { if entry.key != key { continue } sm.entries[i].value = value sm.entries[i].title = title sm.entries[i].level = level sm.entries[i].priority = priority - } - - sm.entries = append(sm.entries, startupMessageEntry{key: key, title: title, value: value, level: level, priority: priority}) + return + } + + sm.entries = append(sm.entries, startupMessageEntry{key: key, title: title, value: value, level: level, priority: priority}) }
🧹 Nitpick comments (3)
services.go (1)
146-150: Remove redundant uppercasing.For the non-error path,
stateis converted to uppercase on line 148, then uppercased again when printing on line 150. Consider removing the uppercasing on line 148 since line 150 already appliesstrings.ToUpper(state)for both paths.Apply this diff to eliminate the redundancy:
} else { stateColor = scheme.Blue - state = strings.ToUpper(state) } fmt.Fprintf(out, "%sINFO%s 🥡 %s[ %s ] %s%s\n", scheme.Green, scheme.Reset, stateColor, strings.ToUpper(state), srv.String(), scheme.Reset)listen.go (1)
374-469: startupMessage / Child PID rendering and hook sequencing look correctKey points in
startupMessage:
- Builds default entries (server address, app name, handlers, prefork status, PID, process count) into
PreStartupMessageDatabefore running pre-startup hooks, allowing hooks to mutate/replace them (ResetEntries,Add*,PreventDefault, customBannerHeader).- Always computes
disabled,isChild, andpreventedand defers construction ofPostStartupMessageData, so post hooks run even when the message is suppressed.- Skips printing when
DisableStartupMessageis true, when running in a prefork child (IsChild()), or when a pre hook setsPreventDefault, matching the new tests.- Child PIDs block prints a single “Child PIDs” header and then formats PIDs in rows of up to 10 per line, which is reasonable for large prefork configurations.
Overall the control flow and output look consistent with the intended API and test coverage; no blocking issues here.
You could optionally factor the Child PIDs inner loop to avoid recomputing
len(listenData.ChildPIDs[start:end])on each iteration by assigningrow := listenData.ChildPIDs[start:end]once and usinglen(row)in the inner loop.hooks.go (1)
181-216: Clone helpers should deep-copy ListenData value, not just reuse the pointerBoth
newPreStartupMessageDataandnewPostStartupMessageDatado:clone := listenData if len(listenData.ChildPIDs) > 0 { clone.ChildPIDs = append([]int(nil), listenData.ChildPIDs...) } return &PreStartupMessageData{ListenData: clone}and similarly for
PostStartupMessageData.Because
cloneis just another pointer to the sameListenDatastruct, assigningclone.ChildPIDs = ...mutates the originallistenData’sChildPIDsfield as well, and all hooks see and share the same underlying struct. If the intent is to provide snapshot-like data to hooks (especially for post-startup), these helpers should deep-copy the struct value and then deep-copy the slice:-func newPreStartupMessageData(listenData *ListenData) *PreStartupMessageData { - clone := listenData - if len(listenData.ChildPIDs) > 0 { - clone.ChildPIDs = append([]int(nil), listenData.ChildPIDs...) - } - - return &PreStartupMessageData{ListenData: clone} -} +func newPreStartupMessageData(listenData *ListenData) *PreStartupMessageData { + if listenData == nil { + return &PreStartupMessageData{} + } + + clone := *listenData + if len(clone.ChildPIDs) > 0 { + clone.ChildPIDs = append([]int(nil), clone.ChildPIDs...) + } + + return &PreStartupMessageData{ListenData: &clone} +} @@ -func newPostStartupMessageData(listenData *ListenData, disabled, isChild, prevented bool) PostStartupMessageData { - clone := listenData - if len(listenData.ChildPIDs) > 0 { - clone.ChildPIDs = append([]int(nil), listenData.ChildPIDs...) - } - - return PostStartupMessageData{ - ListenData: clone, - Disabled: disabled, - IsChild: isChild, - Prevented: prevented, - } -} +func newPostStartupMessageData(listenData *ListenData, disabled, isChild, prevented bool) PostStartupMessageData { + var clonePtr *ListenData + if listenData != nil { + clone := *listenData + if len(clone.ChildPIDs) > 0 { + clone.ChildPIDs = append([]int(nil), clone.ChildPIDs...) + } + clonePtr = &clone + } + + return PostStartupMessageData{ + ListenData: clonePtr, + Disabled: disabled, + IsChild: isChild, + Prevented: prevented, + } +}This keeps hook-visible data stable even if the original
ListenDatais later mutated and avoids surprising cross-hook side effects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/api/hooks.md(1 hunks)docs/whats_new.md(1 hunks)hooks.go(5 hunks)listen.go(5 hunks)listen_test.go(5 hunks)services.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/api/hooks.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/whats_new.md
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-13T11:34:38.503Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3824
File: listen_test.go:560-564
Timestamp: 2025-11-13T11:34:38.503Z
Learning: The gofiber/fiber project only supports the latest Go versions, so Go 1.22+ syntax features (such as `for range N` integer range loops) are acceptable and preferred.
Applied to files:
listen_test.go
📚 Learning: 2025-09-18T00:32:59.671Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 0
File: :0-0
Timestamp: 2025-09-18T00:32:59.671Z
Learning: In Go 1.22, manual copying of loop variables (like `x := x`) is no longer necessary due to the fix for variable capture in closures. Each loop iteration now creates its own instance of the loop variable automatically.
Applied to files:
listen_test.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
listen_test.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.
Applied to files:
hooks.go
🧬 Code graph analysis (3)
listen.go (4)
app.go (2)
App(68-112)Version(37-37)hooks.go (3)
ListenData(74-89)StartupMessageLevelWarning(57-57)StartupMessageLevelError(59-59)prefork.go (1)
IsChild(31-33)color.go (1)
Colors(8-53)
listen_test.go (4)
app.go (1)
New(523-636)listen.go (1)
ListenConfig(42-125)hooks.go (3)
Hooks(33-49)PreStartupMessageData(92-102)PostStartupMessageData(191-202)prefork.go (1)
IsChild(31-33)
hooks.go (3)
color.go (1)
Colors(8-53)app.go (1)
Version(37-37)prefork.go (1)
IsChild(31-33)
⏰ 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: unit (1.25.x, macos-latest)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
- GitHub Check: lint
- GitHub Check: update_release_draft
🔇 Additional comments (8)
services.go (1)
144-144: No issues found —errStringis properly defined.
errStringis defined as a package-level constant athooks.go:62with value"ERROR", which is appropriate for the change on line 144 ofservices.go.listen_test.go (3)
627-662: Good coverage of pre/post startup customization behavior
Test_StartupMessageCustomizationnicely exercises the new API:
- Uses
OnPreStartupMessageto replace the banner and entries (ResetEntries,AddInfo) and verifies the default entries are gone.- Uses
OnPostStartupMessageto validate the disabled/child/prevented flags.The flow matches the implementation in
startupMessageand provides solid regression coverage.
664-684: Disabled startup message + post hook behavior looks correct
Test_StartupMessageDisabledPostHookcorrectly checks that:
- No startup output is emitted when
DisableStartupMessageis true.- The
PostStartupMessageDataflags reflectDisabled=true,IsChild=false,Prevented=false.This aligns with the early-return path in
startupMessagewhile still exercising the deferred post-hook logic.
686-712: PreventDefault hook semantics are validated well
Test_StartupMessagePreventedByHookverifies thePreventDefaultflag:
- Pre hook sets
data.PreventDefault = true.- No startup message is printed.
- Post hook sees
Prevented=truewithDisabled=falseandIsChild=false.This matches the
preventedcalculation and guard instartupMessageand covers an important API usage.listen.go (3)
158-249: Listen/Listener refactor to ListenData pipeline looks consistentThe changes in
ListenandListener:
- Call
app.startupProcess()as before.- Build a
ListenDataviaprepareListenData(ln.Addr().String(), getTLSConfig(ln) != nil, &cfg, nil).- Run
app.runOnListenHooks(&listenData)andapp.printMessages(&cfg, &listenData).This keeps the external behavior while centralizing startup metadata and makes it much easier for hooks/tests to reason about startup state. No functional issues spotted in this part.
Also applies to: 253-288
330-372: prepareListenData correctly normalizes host, TLS, and process metadata
prepareListenData:
- Uses
parseAddrto split host/port and falls back to0.0.0.0or[::1]based onListenerNetwork, matching existing semantics.- Computes
ProcessCountbased onEnablePrefork(usingruntime.GOMAXPROCS(0)), and tracksPID,TLS,Prefork, andHandlerCount.- Clones the
childPIDsslice before storing it inListenDatato avoid aliasing the input slice.This is a clean centralization of all values that feed into the startup banner and hooks.
471-491: printStartupEntries implementation matches lint expectations and simplifies labeling
printStartupEntries:
- Sorts entries by
prioritydescending, then prints them in order.- Computes
label/coloronce per entry via a small switch and uses a singlefmt.Fprintf, eliminating duplicated branches that lint flagged previously.This is concise and easier to extend (e.g., new levels) without duplicating format strings.
hooks.go (1)
73-89: Startup hook surface and wiring look solidThe new pieces:
ListenDatanow carriesColorScheme, app/version metadata, process counts, PID, TLS/Prefork, andChildPIDs, which is sufficient for startup-related hooks.PreStartupMessageDataandPostStartupMessageDataembed*ListenDataand expose:
- Banner control (
BannerHeader),- Entry management (
AddInfo/AddWarning/AddError,EntryKeys,ResetEntries,DeleteEntry),- Control flags (
PreventDefault,Disabled,IsChild,Prevented).- Hooks are registered via
OnPreStartupMessage/OnPostStartupMessageand executed inexecuteOnPreStartupMessageHooks/executeOnPostStartupMessageHookswith short-circuit on first error.executeOnListenHooksnow takes a*ListenDataand still passes a value intoOnListenHandler, preserving the public handler signature.The overall API is coherent, aligns with the tests in
listen_test.go, and offers a flexible customization surface without breaking existing hook behavior.Also applies to: 91-132, 190-201, 218-290, 396-424
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hooks.go(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.
Applied to files:
hooks.go
🧬 Code graph analysis (1)
hooks.go (3)
color.go (1)
Colors(8-53)app.go (1)
Version(37-37)prefork.go (1)
IsChild(31-33)
🔇 Additional comments (10)
hooks.go (10)
18-21: LGTM: Handler types are well-defined.The new handler signatures are appropriate. The
OnPreStartupMessageHandlertakes a pointer to allow mutation of startup message data, whileOnPostStartupMessageHandlertakes a value for read-only access post-printing.
51-62: LGTM: Enum definition follows Go conventions.The
StartupMessageLevelenum and constants are well-structured. The levels (Info, Warning, Error) provide appropriate categorization for startup messages.
73-89: LGTM: ListenData appropriately extended.The new fields provide comprehensive metadata for startup message customization, mixing presentation data (ColorScheme, AppName) with runtime information (PID, HandlerCount, ChildPIDs).
91-102: LGTM: PreStartupMessageData structure is sound.The struct appropriately embeds
*ListenData, provides banner customization viaBannerHeader, manages entries privately, and allows suppressing the default message withPreventDefault. The field order appears to address the alignment concerns from previous reviews.
104-132: LGTM: Entry addition methods are consistent and well-designed.The
AddInfo,AddWarning, andAddErrormethods provide a clean API with optional priority. The variadic priority parameter with a default of -1 is a good design choice.
134-160: LGTM: Entry management methods are correctly implemented.The methods handle their responsibilities well:
EntryKeyssafely returns a new sliceResetEntriesuses the idiomatic[:0]patternDeleteEntryproperly handles nil checks and removes the first matching entry
162-186: LGTM: Duplicate entry issue resolved.The
addEntrymethod now correctly returns after updating an existing entry (line 176), preventing the duplicate entry bug that was flagged in previous reviews. The logic properly handles both update and insert cases.
225-240: LGTM: Hook initialization is consistent.The
newHooksfunction properly initializes the newonPreStartupandonPostStartupslices alongside existing hooks.
285-297: LGTM: Hook registration methods follow established patterns.Both
OnPreStartupMessageandOnPostStartupMessagecorrectly implement thread-safe registration using mutexes and support variadic handler arguments for convenience.
403-431: LGTM: Hook execution methods are correctly implemented.The execution methods properly match their handler signatures:
executeOnListenHooksdereferences the pointer for value-based handlersexecuteOnPreStartupMessageHookspasses the pointer for mutable accessexecuteOnPostStartupMessageHookspasses the value for read-only accessError propagation is consistent across all three methods.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/api/hooks.md(1 hunks)docs/whats_new.md(1 hunks)hooks.go(5 hunks)listen.go(5 hunks)listen_test.go(5 hunks)services.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- listen_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.
Applied to files:
docs/whats_new.mdhooks.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
docs/whats_new.md
🧬 Code graph analysis (2)
listen.go (4)
app.go (2)
App(68-112)Version(37-37)hooks.go (1)
ListenData(74-89)prefork.go (1)
IsChild(31-33)color.go (1)
Colors(8-53)
hooks.go (3)
color.go (1)
Colors(8-53)app.go (1)
Version(37-37)prefork.go (1)
IsChild(31-33)
⏰ 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). (2)
- GitHub Check: repeated
- GitHub Check: Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
hooks.go (2)
188-190: Fix shallow-copy bug: original ListenData can be mutated.The constructor directly embeds the pointer without copying the struct. If a hook modifies fields in
PreStartupMessageData.ListenData, it will mutate the originalListenData, breaking encapsulation and causing unexpected side effects.Apply this diff to create a proper copy:
func newPreStartupMessageData(listenData *ListenData) *PreStartupMessageData { - return &PreStartupMessageData{ListenData: listenData} + clone := *listenData + if len(listenData.ChildPIDs) > 0 { + clone.ChildPIDs = append([]int(nil), listenData.ChildPIDs...) + } + return &PreStartupMessageData{ListenData: &clone} }Based on learnings from past review comments.
206-218: Fix shallow-copy bug: ChildPIDs slice shares underlying array.Line 209 uses
clone.ChildPIDs = clone.ChildPIDs[:]which creates a new slice header but still references the same underlying array. Modifications to the cloned slice's elements will affect the originalListenData.Apply this diff to properly deep-copy the slice:
func newPostStartupMessageData(listenData *ListenData, disabled, isChild, prevented bool) PostStartupMessageData { clone := *listenData if len(listenData.ChildPIDs) > 0 { - clone.ChildPIDs = clone.ChildPIDs[:] // make a copy + clone.ChildPIDs = append([]int(nil), listenData.ChildPIDs...) } return PostStartupMessageData{ ListenData: &clone, Disabled: disabled, IsChild: isChild, Prevented: prevented, } }Based on learnings from past review comments.
🧹 Nitpick comments (1)
hooks.go (1)
91-102: Note: Past review flagged field alignment issue.A previous review suggested reordering fields to reduce padding waste. If the linter still flags this, consider reordering as:
entries,BannerHeader,*ListenData,PreventDefault.Based on learnings from past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/api/hooks.md(1 hunks)docs/whats_new.md(1 hunks)hooks.go(5 hunks)hooks_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/api/hooks.md
- docs/whats_new.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.
Applied to files:
hooks.go
🧬 Code graph analysis (2)
hooks_test.go (3)
app.go (3)
New(523-636)Config(115-426)Version(37-37)listen.go (1)
ListenConfig(42-125)hooks.go (3)
Hooks(33-49)ListenData(74-89)PreStartupMessageData(92-102)
hooks.go (3)
color.go (1)
Colors(8-53)app.go (1)
Version(37-37)prefork.go (1)
IsChild(31-33)
⏰ 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: Compare
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: lint
- GitHub Check: update_release_draft
🔇 Additional comments (13)
hooks_test.go (4)
6-7: LGTM!The new imports support validation of PID and process count metadata in the startup message tests.
292-352: LGTM!Comprehensive test coverage for ListenData metadata and PreStartupMessageData manipulation. Validates all fields, hook execution, and entry management correctly.
354-492: LGTM!Thorough test coverage of all PreStartupMessageData helper functions. The AddInfo-UpdateExisting subtest correctly verifies that duplicate keys update the existing entry rather than creating duplicates.
626-626: LGTM!Correctly updated to pass a pointer to ListenData, matching the new
executeOnListenHooks(*ListenData)signature.hooks.go (9)
18-21: LGTM!The handler signatures are correctly designed:
OnPreStartupMessageHandlerreceives a pointer to allow mutation of the startup message, whileOnPostStartupMessageHandlerreceives a value for read-only access.
43-44: LGTM!The new hook slices follow the established pattern in the Hooks struct.
51-62: LGTM!The StartupMessageLevel enumeration follows standard Go patterns.
64-71: LGTM!The internal
startupMessageEntrystruct is appropriately designed for storing startup message data.
73-89: LGTM!The expanded ListenData struct provides comprehensive metadata for startup hooks.
104-132: LGTM!The Add* methods provide a clean, consistent API for adding startup message entries with optional priority.
134-160: LGTM!The helper methods are correctly implemented.
ResetEntriesefficiently clears the slice while preserving capacity.
162-186: LGTM!The
addEntrymethod correctly returns after updating an existing entry (line 176), preventing duplicate entries. This addresses the issue flagged in previous reviews.
228-229: LGTM!The hook registration and execution methods are correctly implemented:
- Mutex locking protects concurrent access during registration
executeOnListenHooksdereferences the pointer to pass a copy to handlers (line 400), preventing mutationexecuteOnPreStartupMessageHookspasses a pointer for mutation (line 410)executeOnPostStartupMessageHookspasses a value for read-only access (line 420)Also applies to: 280-292, 398-426
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
hooks_test.go (1)
354-519: Consider reducing white‑box coupling and strengthening AddWarning/AddError coverageThe helper-function subtests give nice coverage for
EntryKeys,ResetEntries,AddInfo,AddWarning,AddError, update, and delete behavior. Two optional refinements:
- The tests reach into
data.entriesand index into the slice directly. Sinceentriesis an internal detail, you may want to assert via public helpers (e.g.,EntryKeysplus a tiny lookup helper) to reduce coupling to the internal slice layout and ordering.- For
AddWarningandAddError, you currently assert only key/title/value/priority, which is identical toAddInfofrom the test’s point of view. IfstartupMessageEntrycarries a message kind/severity, consider also asserting that the kind changes appropriately so regressions where warning/error fall back to info are caught.These are minor and can be deferred, but would make the tests more robust against internal refactors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hooks_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
hooks_test.go (3)
app.go (3)
New(523-636)Config(115-426)Version(37-37)listen.go (1)
ListenConfig(42-125)hooks.go (4)
Hooks(33-49)ListenData(74-89)PreStartupMessageData(92-102)PostStartupMessageData(193-204)
⏰ 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: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: lint
- GitHub Check: Compare
- GitHub Check: update_release_draft
🔇 Additional comments (3)
hooks_test.go (3)
6-7: New imports are correctly scoped to metadata tests
osandruntimeare used only in the new metadata test to assert PID and process count; the additions are appropriate and localized to tests.
292-352: ListenData/PreStartup metadata test looks solid
Test_ListenDataMetadataexercisesprepareListenData,OnListen, andOnPreStartupMessageagainst the sameListenDatainstance, validating host/port, TLS, app name, handler count, process count, PID, prefork, child PIDs, and color scheme. This gives good coverage of the new metadata fields and pointer-based flow without introducing brittle assumptions.
645-679: Updated listen and startup hook error-path tests align with new APIs
Test_executeOnListenHooks_Errornow passes a*ListenDataintoexecuteOnListenHooks, matching the updated signature while still asserting error propagation.- The new
Test_executeOnPreStartupMessageHooks_ErrorandTest_executeOnPostStartupMessageHooks_Errorverify that failures from pre/post-startup message hooks are surfaced by their respective executors.These tests correctly exercise the error paths for the new hook APIs without introducing extra complexity.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
hooks.go (1)
20-23: Consider making handler signatures consistent.
OnPreStartupMessageHandleraccepts a pointer (*PreStartupMessageData) whileOnPostStartupMessageHandleraccepts a value (PostStartupMessageData). The pointer allows pre-hooks to mutate startup data, but the asymmetry may confuse users. If post-hooks don't need mutation, document why; if both should be read-only after the fact, consider passing*PostStartupMessageDatafor symmetry with other hook pairs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hooks.go(6 hunks)hooks_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- hooks_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.
Applied to files:
hooks.go
🧬 Code graph analysis (1)
hooks.go (3)
color.go (1)
Colors(8-53)app.go (1)
Version(37-37)prefork.go (1)
IsChild(31-33)
⏰ 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). (3)
- GitHub Check: repeated
- GitHub Check: Compare
- GitHub Check: update_release_draft
🔇 Additional comments (6)
hooks.go (6)
34-91: LGTM! Clean additions to support startup message hooks.The new hook slices, StartupMessageLevel enum, startupMessageEntry internal struct, and expanded ListenData all follow existing patterns and provide a clear API surface for startup customization.
106-188: LGTM! Entry manipulation methods are correct.The AddInfo/AddWarning/AddError methods properly handle optional priority, and the addEntry method now correctly returns after updating an existing entry (line 178), fixing the previous duplicate-entry bug. EntryKeys, ResetEntries, and DeleteEntry all use standard Go patterns.
190-192: Verify: Should PreStartupMessageData copy ListenData?
newPreStartupMessageDatapasses the original*ListenDatapointer directly, allowing pre-hooks to mutate the original struct. In contrast,newPostStartupMessageData(lines 208-220) creates a defensive deep copy withclone := *listenData.If pre-hooks should mutate shared state, the current design is correct. If they should receive isolated copies (similar to post-hooks), apply this pattern:
func newPreStartupMessageData(listenData *ListenData) *PreStartupMessageData { clone := *listenData if len(listenData.ChildPIDs) > 0 { clone.ChildPIDs = slices.Clone(listenData.ChildPIDs) } return &PreStartupMessageData{ListenData: &clone} }
222-294: LGTM! Hook initialization and registration follow established patterns.The newHooks constructor properly initializes the new hook slices, and OnPreStartupMessage/OnPostStartupMessage registration methods correctly use mutex locking consistent with other hook methods.
400-428: LGTM! Hook execution methods correctly match handler signatures.
executeOnListenHooksnow accepts a pointer and dereferences it to match the value-basedOnListenHandlersignature. The newexecuteOnPreStartupMessageHooksandexecuteOnPostStartupMessageHookscorrectly propagate pointer and value types respectively, matching their handler signatures.
93-104: No alignment issues detected—struct already satisfies field alignment requirements.Verification using
go vet -vet=allfound no field alignment violations inPreStartupMessageData. The current field ordering does not trigger alignment warnings from Go's standard tooling. The concern raised in the original review is not present in the codebase.
|
@efectn thoughts on this: hooks.go (1) OnPreStartupMessageHandler accepts a pointer (*PreStartupMessageData) while OnPostStartupMessageHandler accepts a value (PostStartupMessageData). The pointer allows pre-hooks to mutate startup data, but the asymmetry may confuse users. If post-hooks don't need mutation, document why; if both should be read-only after the fact, consider passing *PostStartupMessageData for symmetry with other hook pairs |
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces an excellent feature for customizing startup messages, providing a much-improved user experience. The new hooks OnPreStartupMessage and OnPostStartupMessage are well-designed, and the API for adding, removing, and modifying startup message entries is intuitive. The associated refactoring to centralize listener data into the ListenData struct and pass it around by pointer has cleaned up the code in listen.go and prefork.go significantly. The documentation updates are comprehensive, and the new tests cover the functionality well. I have a couple of minor suggestions for further improvement, but overall, this is a high-quality contribution.
| var clonedPIDs []int | ||
| if len(childPIDs) > 0 { | ||
| clonedPIDs = append(clonedPIDs, childPIDs...) | ||
| } |
There was a problem hiding this comment.
For cloning the childPIDs slice, you can use slices.Clone(childPIDs) which is more idiomatic since Go 1.21. This would make the code more concise and align with modern Go practices. You'll need to add "slices" to the imports.
| var clonedPIDs []int | |
| if len(childPIDs) > 0 { | |
| clonedPIDs = append(clonedPIDs, childPIDs...) | |
| } | |
| clonedPIDs := slices.Clone(childPIDs) |
| app.logServices(app.servicesStartupCtx(), out, &colors) | ||
|
|
||
| if listenData.Prefork && len(listenData.ChildPIDs) > 0 { | ||
| fmt.Fprintf(out, "%sINFO%s Child PIDs: \t\t%s", colors.Green, colors.Reset, colors.Blue) |
There was a problem hiding this comment.
The INFO label is hardcoded here. For consistency with printStartupEntries, it would be better to use the same mechanism to generate the label and color based on the message level. This would also make it easier to change the label style globally in the future.
There was a problem hiding this comment.
@efectn are we forcing colors here? It should use the bool from the config to determine if colors are needed
There was a problem hiding this comment.
It is not about colors. Prefork message is not added to startupMessageEntry like other messages. It should use app colors with no problem. Maybe i can add it to startupmessageentry just like other messages if it is necessary
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ReneWerner87
left a comment
There was a problem hiding this comment.
looks good, can you check my comments
|
|
||
| `MIMEApplicationJavaScript` and `MIMEApplicationJavaScriptCharsetUTF8` are deprecated. Use `MIMETextJavaScript` and `MIMETextJavaScriptCharsetUTF8` instead. | ||
|
|
||
| ## 🎣 Hooks |
There was a problem hiding this comment.
pls extend this section with the 2 new hooks
|
@efectn can you check my hints |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
hooks.go (2)
53-74: Pre-startup entry model is well-structured; fix minor doc mismatchThe
StartupMessageLevelenum,startupMessageEntry, andPreStartupMessageDatahelpers (AddInfo,AddWarning,AddError,EntryKeys,ResetEntries,DeleteEntry,addEntry) are implemented correctly:
- Optional
prioritydefaults to-1, allowing ordering without forcing a value.- Re-adding the same key updates in place and avoids duplicates.
ResetEntries/DeleteEntrysafely handle nil and non-nil slices.One small nit: the
AddWarningcomment says it adds an entry with"WARNING"label, butprintStartupEntriesrenders the label as"WARN". Consider aligning the comment with the actual label (or vice versa) to avoid confusion.-// AddWarning adds a warning entry to the startup message with "WARNING" label. +// AddWarning adds a warning entry to the startup message with a "WARN" label.Also applies to: 93-188
190-220: Consider documenting or aligning Pre/Post ListenData mutability
newPreStartupMessageDataexposes the original*ListenDatato pre-startup hooks, whilenewPostStartupMessageDatacreates a struct copy and clonesChildPIDsbefore handing data to post-startup hooks. That means:
- Pre hooks can mutate the original
ListenData(and those changes are then visible to post hooks).- Post hooks work on an isolated copy and cannot affect internal state.
This asymmetry is probably intentional (pre hooks can influence rendering; post hooks are observational only), but it’s subtle. I’d either:
- Add a brief comment to both constructors explaining the intended mutability semantics, or
- Make
newPreStartupMessageDatamirrornewPostStartupMessageDataby cloningListenDataas well, if you prefer full isolation.-func newPreStartupMessageData(listenData *ListenData) *PreStartupMessageData { - return &PreStartupMessageData{ListenData: listenData} -} +// newPreStartupMessageData constructs a pre-startup payload. +// It intentionally exposes the original ListenData so hooks can +// influence rendering (e.g., via PreventDefault or custom entries). +func newPreStartupMessageData(listenData *ListenData) *PreStartupMessageData { + return &PreStartupMessageData{ListenData: listenData} +}(If you’d rather isolate pre hooks as well, mirror the
newPostStartupMessageDatacloning logic instead.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/api/hooks.md(3 hunks)hooks.go(6 hunks)hooks_test.go(3 hunks)listen.go(5 hunks)listen_test.go(5 hunks)prefork.go(3 hunks)prefork_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/api/hooks.md
- listen_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.
Applied to files:
hooks.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
prefork_test.go
🧬 Code graph analysis (4)
hooks_test.go (3)
app.go (2)
Config(115-426)Version(37-37)listen.go (1)
ListenConfig(43-126)hooks.go (4)
Hooks(35-51)ListenData(76-91)PreStartupMessageData(94-104)PostStartupMessageData(195-206)
listen.go (4)
app.go (2)
App(68-112)Version(37-37)hooks.go (1)
ListenData(76-91)prefork.go (1)
IsChild(31-33)color.go (1)
Colors(8-53)
hooks.go (3)
color.go (1)
Colors(8-53)app.go (1)
Version(37-37)prefork.go (1)
IsChild(31-33)
prefork_test.go (1)
app.go (1)
New(523-636)
⏰ 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). (4)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: lint
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (11)
prefork.go (1)
95-123: Prefork child PID propagation into ListenData looks correctCollecting
childPIDsas[]intand feeding them intoprepareListenData/startupMessageintegrates prefork metadata cleanly into the new startup flow. Cloning happens insideprepareListenData, so the master’s slice isn’t exposed, and hooks/startup message now have access toChildPIDswithout changing existing prefork behavior.Also applies to: 139-146
hooks_test.go (4)
3-16: New imports are justified by added testsThe added
osandruntimeimports are used in metadata assertions and are scoped to tests, so there’s no production impact.
292-519: ListenData and PreStartupMessageData helper tests provide solid coverageThese tests thoroughly validate that
prepareListenDatapopulates host/port/TLS/version/app name/prefork/child PIDs/handler count/process count/PID/color scheme correctly and that the pre-startup helpers (EntryKeys,ResetEntries,AddInfo/AddWarning/AddError,DeleteEntry, and overwrite semantics) behave as intended. This gives good confidence in the new hook API surface.
645-655: executeOnListenHooks pointer change is correctly exercisedSwitching
executeOnListenHooksto accept*ListenDataand passing&ListenData{...}in the test keeps the public handler signature (func(ListenData)) unchanged while ensuring handlers receive a value copy. The error assertion confirms propagation still works.
657-679: Pre/Post startup hook error paths are well coveredThe new tests for
executeOnPreStartupMessageHooks_ErrorandexecuteOnPostStartupMessageHooks_Errorcorrectly verify that the first handler error is returned. This aligns the behavior withexecuteOnListenHooksand ensures callers can detect hook failures when they choose to.listen.go (3)
165-175: Single ListenData pipeline for Listen/Listener is clean and consistentCreating
listenDataonce viaprepareListenData, then feeding it throughrunOnListenHooksandprintMessagesfor bothListenandListenerremoves duplication and keeps all startup metadata (including TLS and prefork) in one place. Callers still see the same behavior, and config is passed by pointer throughout, so future fields will naturally propagate.Also applies to: 208-223, 225-241
331-373: prepareListenData correctly normalizes address and isolates ChildPIDs
prepareListenDataproperly:
- Normalizes host/port, defaulting to
0.0.0.0or[::1]when the host is empty.- Derives
ProcessCountfromGOMAXPROCSonly when prefork is enabled.- Clones
childPIDsviaslices.Clone, so internalListenData.ChildPIDscan’t accidentally mutate the caller’s slice.- Populates handler count, PID, TLS, and prefork flags.
This is a good internal seam for both prefork and non-prefork flows.
375-467: startupMessage/printStartupEntries implement the new hookable startup banner correctlyThe new
startupMessageflow looks sound:
- Builds sensible defaults (address, app name, handlers, prefork, PID, process count) into
PreStartupMessageData.- Runs pre-startup hooks before rendering, allowing hooks to reset or override entries and the banner (
BannerHeader) and to setPreventDefault.- Honors
DisableStartupMessage, child-process detection (IsChild()), andPreventDefaultto skip printing while still always executing post-startup hooks viadefer.- Renders entries with a single
printStartupEntriesimplementation that maps levels to labels/colors, and logs services and prefork child PIDs in a structured way.The separation of concerns between data preparation, hooks, and rendering is clear and matches the proposed API.
Also applies to: 472-492
prefork_test.go (1)
78-100: Child-process “no startup message” test matches the new startup pipelineSwitching the test to call
startupProcess(), thenprepareListenDataandstartupMessagemirrors the actual prefork child flow while still asserting that nothing is printed whenIsChild()is true. This keeps the regression check valid after the API refactor.hooks.go (2)
9-51: New startup hook handler types and Hooks fields are consistentAdding
OnPreStartupMessageHandler/OnPostStartupMessageHandlerand wiring them intoHooksalongside the existing hook types, with registration methods that lockh.app.mutex, fits the existing hook model and keeps handler signatures straightforward (pointers for pre/post startup data, value forListenData).
222-236: Hook initialization and execute implementations follow existing patterns*
newHookscorrectly initializes the newonPreStartupandonPostStartupslices, and the execute methods:
executeOnListenHooks(*ListenData)now take a pointer but still pass a value into handlers, preserving the publicOnListenHandlerAPI while avoiding accidental mutation.executeOnPreStartupMessageHooks/executeOnPostStartupMessageHooksiterate handlers and return on the first error, matching howexecuteOnListenHooksbehaves and enabling callers (like tests andstartupMessage) to decide how to handle failures.This is consistent with the rest of the hook subsystem.
Also applies to: 400-428
Description
This PR is successor of previous #3802 PR of @gaby to improve UX. Different than old PR, it gives ability to add messages with more user-friendly API like:
Fixes #3800
Changes introduced
Type of change
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md