Conversation
|
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. WalkthroughConsolidates startup messaging around a new ListenData struct, adds OnPreStartupMessage and OnPostStartupMessage hooks with PreStartupMessageData/PostStartupMessageData, refactors prepareListenData/startupMessage/printMessages to accept ListenData (child PIDs as []int), updates tests/docs, and removes the return value from startupProcess. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Hooks
participant Renderer as StartupMessage
App->>App: prepareListenData(addr, isTLS, cfg, childPIDs)
App->>Hooks: executeOnPreStartupMessageHooks(*PreStartupMessageData)
alt PreventDefault or DisableStartupMessage
Hooks-->>App: PreventDefault=true / Disabled=true
note over App,Renderer #f6f8ff: Skip default rendering
else Render default banner
App->>Renderer: Render using ListenData (Host,Port,Version,PID,ChildPIDs,...)
Renderer-->>App: Printed
end
App->>Hooks: executeOnPostStartupMessageHooks(PostStartupMessageData)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3802 +/- ##
==========================================
+ Coverage 91.73% 92.03% +0.30%
==========================================
Files 113 114 +1
Lines 11966 12259 +293
==========================================
+ Hits 10977 11283 +306
+ Misses 727 712 -15
- Partials 262 264 +2
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:
|
|
/gemini review |
There was a problem hiding this comment.
Pull Request Overview
This PR enriches the ListenData structure with comprehensive metadata and introduces new hooks for customizing the startup banner message. The changes enable developers to modify or suppress the default Fiber startup message and receive notifications about the message's display status.
Key changes:
- Expanded
ListenDatawith version, app name, process counts, PIDs, and color scheme metadata - Added
OnPreStartupMessageandOnPostStartupMessagehooks for startup banner customization - Refactored startup message generation to use the enriched data structures across both standard and prefork modes
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| hooks.go | Defines new startup message data structures and hook handlers with metadata propagation |
| listen.go | Updates startup message generation to use enriched ListenData and new hook system |
| prefork.go | Refactors prefork startup to use consistent ListenData approach with child PID tracking |
| app.go | Simplifies startupProcess method signature by removing return value |
| hooks_test.go | Adds comprehensive tests for new metadata fields and hook functionality |
| listen_test.go | Updates existing tests and adds new tests for startup message customization features |
| prefork_test.go | Updates test to use new startupMessage method signature |
| docs/ | Documents the new startup message customization capabilities and expanded ListenData |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for customizing the startup message in Fiber. The changes are well-structured, and the refactoring to centralize listener metadata in ListenData improves code clarity. The new hooks OnPreStartupMessage and OnPostStartupMessage provide great flexibility. The documentation and tests are comprehensive and clearly explain the new functionality. I've added a few minor suggestions to improve code conciseness and remove some redundant checks. Overall, this is an excellent contribution.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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(5 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: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
listen_test.gohooks_test.goapp.goprefork_test.gohooks.golisten.goprefork.go
🧬 Code graph analysis (5)
listen_test.go (3)
app.go (1)
Map(43-43)listen.go (1)
ListenConfig(42-125)hooks.go (3)
Hooks(36-52)PreStartupMessageData(79-101)PostStartupMessageData(125-144)
hooks_test.go (2)
app.go (2)
Version(37-37)Map(43-43)hooks.go (3)
Hooks(36-52)ListenData(61-76)PreStartupMessageData(79-101)
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(61-76)prefork.go (1)
IsChild(31-33)color.go (1)
Colors(8-53)
🔇 Additional comments (32)
app.go (1)
1328-1336: LGTM!The removal of the return value aligns with the refactored startup flow that now operates on
ListenDataobjects. The function still performs all necessary startup operations (locking, mounting, tree building) and the change is consistent across the codebase.docs/api/hooks.md (1)
171-227: LGTM!The documentation clearly explains the new
ListenDatafields and startup message customization hooks. The examples demonstrate proper usage ofOnPreStartupMessageandOnPostStartupMessagehooks, including how to customize the banner, prevent default output, and handle post-startup logic.hooks_test.go (1)
292-342: LGTM!The test comprehensively validates the new
ListenDatametadata fields including version, app name, process counts, PIDs, and color scheme. It properly exercises bothOnListenandOnPreStartupMessagehooks and confirms that customPrimaryInfoandSecondaryInfocan be set and retrieved.prefork_test.go (1)
86-90: LGTM!The test correctly adapts to the new data-driven startup flow by calling
prepareListenDatato build theListenDataobject before passing it tostartupMessage. This aligns with the refactored startup message architecture.listen_test.go (7)
526-544: LGTM!The test correctly builds
childPIDsas[]intand usesprepareListenDatato create theListenDataobject before passing it tostartupMessage. This aligns with the refactored startup flow.
560-573: LGTM!The test properly constructs
ListenDataviaprepareListenDataand validates the startup message with the app name.
591-597: LGTM!The test correctly handles non-ASCII app names and uses the new
ListenData-based flow.
614-625: LGTM!The test validates the startup message with custom endpoints and disabled prefork using the new data structures.
627-657: LGTM!The test validates startup message customization via hooks, including custom header,
PrimaryInfo, andSecondaryInfomaps. It correctly verifies that custom values appear in the output and that thePostStartupMessageDatareflects the printed state.
659-681: LGTM!The test correctly validates that setting
PreventDefault = truesuppresses the startup message and thatPostStartupMessageDatareflects the prevented state.
683-701: LGTM!The test validates that when
DisableStartupMessageis set, no message is printed andPostStartupMessageDatacorrectly indicates the disabled state.prefork.go (2)
93-119: LGTM!The change from
pids []stringtochildPIDs []intis a more type-safe approach. Process IDs are inherently integers, and this change aligns with theListenData.ChildPIDs []intfield.
138-142: LGTM!The startup flow correctly builds
listenDataviaprepareListenData, executes listen hooks, and calls the newstartupMessagesignature with theListenDataobject. This is consistent with the refactored startup message architecture.listen.go (6)
233-239: LGTM!The refactor correctly uses
prepareListenDatato build theListenDataobject and passes it to bothrunOnListenHooksandprintMessages. This centralizes startup metadata in a single structured object.
267-273: LGTM!The
Listenermethod follows the same pattern asListen, correctly buildingListenDataand passing it through the startup flow.
327-333: LGTM!The refactored
printMessagesnow acceptsListenDatainstead of anet.Listener, which is more appropriate since it decouples the printing logic from the listener implementation.
335-369: LGTM!The
prepareListenDatafunction properly constructs theListenDataobject with all necessary fields including host, port, version, app name, handler count, process count, PID, TLS status, prefork status, and child PIDs. The logic for determining process count and cloning child PIDs is correct.
371-476: LGTM!The refactored
startupMessagefunction properly:
- Executes pre-startup hooks to allow customization
- Determines whether to print based on disabled/prevented/child flags
- Executes post-startup hooks with the correct state
- Renders customized or default startup message using
ListenDatafields- Handles both custom info maps and default rendering paths
The deferred execution of post-hooks ensures they run even if the function returns early.
478-482: LGTM!The
printStartupEntrieshelper function cleanly separates the rendering logic for custom startup info entries.hooks.go (13)
4-6: LGTM!The new imports (
fmtandsort) are necessary for formatting startup entries and sorting map keys inmapToEntries.
21-24: LGTM!The new hook handler types
OnPreStartupMessageHandlerandOnPostStartupMessageHandlerfollow the existing hook pattern and provide appropriate signatures for pre- and post-startup customization.
46-47: LGTM!The new hook slices are properly integrated into the
Hooksstructure.
54-58: LGTM!The unexported
startupMessageEntrytype appropriately represents a key-value pair for rendering startup information.
60-76: LGTM!The expanded
ListenDatastructure includes all necessary runtime metadata fields. The use of[]intforChildPIDsis type-safe and appropriate. The structure provides comprehensive information about the listener state.
78-101: LGTM!The
PreStartupMessageDatastructure provides all necessary fields for hook customization, including:
- Custom info maps (
PrimaryInfo,SecondaryInfo)- Custom header with
HeaderSetflag- All listener metadata from
ListenData- Control flags (
PreventDefault,HeaderSet)This enables comprehensive customization of the startup message.
103-122: LGTM!The
newPreStartupMessageDataconstructor properly clones child PIDs and copies all fields fromListenDatato create aPreStartupMessageDatainstance. The slice cloning prevents unintended mutations.
124-144: LGTM!The
PostStartupMessageDatastructure includes all listener metadata plus status flags (Printed,Disabled,Prevented,IsChild) that inform hooks about the startup message lifecycle.
146-169: LGTM!The
newPostStartupMessageDataconstructor properly clones child PIDs and sets all status flags to inform post-startup hooks about what occurred during startup message rendering.
171-189: LGTM!The
mapToEntrieshelper function correctly converts aMapto sorted key-value entries for consistent rendering. The function returns(entries, false)when the map is empty, allowing callers to distinguish between empty and populated maps.
199-200: LGTM!The new hook slices are properly initialized in the
newHooksconstructor.
251-263: LGTM!The new hook registration methods
OnPreStartupMessageandOnPostStartupMessagefollow the existing pattern with proper mutex locking to ensure thread-safe hook registration.
367-377: LGTM!The hook execution methods properly iterate through registered handlers and invoke them with the appropriate data. Unlike other hooks that return errors, these hooks don't return errors, which is appropriate since startup message rendering is informational and shouldn't block the application startup.
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for customizing the startup message through OnPreStartupMessage and OnPostStartupMessage hooks, and enriches ListenData with more metadata. The implementation is well-structured, particularly the refactoring in listen.go and prefork.go which improves code clarity by using the new data structures. The changes are also well-covered by tests. I've identified a minor documentation error in an example and a couple of small opportunities to simplify the code and adhere more closely to idiomatic Go practices.
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
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)
526-544: Edge-case coverage gap for Child PIDsThe test feeds a 60-element Child PID slice (6 values × 10 copies) but only asserts that the banner contains the first 6 values. If the formatting loop broke after the first chunk, this assertion would still pass. Please add expectations for the second (or last) chunk so the multi-row rendering is exercised.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/api/hooks.md(1 hunks)docs/whats_new.md(1 hunks)hooks.go(6 hunks)listen.go(5 hunks)listen_test.go(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/api/hooks.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
listen_test.golisten.gohooks.go
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/whats_new.md
🧬 Code graph analysis (3)
listen_test.go (4)
app.go (2)
New(515-628)Map(43-43)listen.go (1)
ListenConfig(42-125)hooks.go (3)
Hooks(36-52)PreStartupMessageData(79-99)PostStartupMessageData(123-140)prefork.go (1)
IsChild(31-33)
listen.go (4)
app.go (2)
App(68-110)Version(37-37)hooks.go (1)
ListenData(61-76)prefork.go (1)
IsChild(31-33)color.go (1)
Colors(8-53)
hooks.go (3)
color.go (1)
Colors(8-53)app.go (2)
Version(37-37)Map(43-43)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: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (1)
listen_test.go (1)
627-655: Validate PreventDefault behaviorThis customization test never sets
PreventDefault, so we don't confirm that the pre-hook can suppress the default banner entirely. Consider adding a second case (or extending this one) that flipsPreventDefault = trueand asserts the banner is empty to guard that path.
|
|
||
| entries := make([]startupMessageEntry, 0, len(values)) | ||
| for _, key := range keys { | ||
| entries = append(entries, startupMessageEntry{key: key, value: fmt.Sprint(values[key])}) |
There was a problem hiding this comment.
Maybe it could be better to use utils.ToString here
| app.Hooks().OnPreStartupMessage(func(sm *fiber.PreStartupMessageData) { | ||
| sm.Header = "FOOBER " + sm.Version + "\n-------" | ||
| sm.PrimaryInfo = fiber.Map{"Git hash": os.Getenv("GIT_HASH")} | ||
| sm.SecondaryInfo = fiber.Map{"Process count": sm.ProcessCount} |
There was a problem hiding this comment.
I think it should be possible to add additional informations to secondary info without overriding current info. Maybe predefine it with info like prefork/pid etc. and allow user to remove them or customize as they wish
There was a problem hiding this comment.
When we set SecondaryInfo, the prefork/pid info is lost from the startup message. I think by default we should set SecondaryInfo. Instead we should set SecondaryInfo before the hook like
sm.SecondaryInfo = fiber.Map{"pid": ..., "processes": ...} using the messages here SecondaryInfo. So when user wants to add new information without deletion prefork etc. they can just use sm.SecondaryInfo["Test"] = "..."
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
hooks.go (4)
60-76: Consider adding betteralign directive with explanation.The
ListenDatastruct fields are ordered for logical grouping and API clarity rather than memory alignment. Consider adding a//betteralign:ignoredirective with a comment explaining this intentional choice, as suggested in previous reviews.Apply this diff:
+// ListenData contains the listener metadata provided to OnListenHandler. +// +//betteralign:ignore // Field order prioritizes logical grouping and public API documentation over memory alignment -// ListenData contains the listener metadata provided to OnListenHandler. type ListenData struct {
92-94: Simplify slice cloning.The length check before cloning is unnecessary. The
appendfunction handles empty ornilslices correctly, making the code more concise.Apply this diff:
- if len(listenData.ChildPIDs) > 0 { - clone.ChildPIDs = append([]int(nil), listenData.ChildPIDs...) - } + clone.ChildPIDs = append([]int(nil), listenData.ChildPIDs...)
110-112: Simplify slice cloning.Same issue as in
newPreStartupMessageData- the length check before cloning is unnecessary.Apply this diff:
- if len(listenData.ChildPIDs) > 0 { - clone.ChildPIDs = append([]int(nil), listenData.ChildPIDs...) - } + clone.ChildPIDs = append([]int(nil), listenData.ChildPIDs...)
136-136: Consider using utils.ToString for value formatting.As suggested in a previous review,
utils.ToStringmight provide better type handling or be more idiomatic for this codebase thanfmt.Sprint.Verify whether
utils.ToStringexists and would be more appropriate:#!/bin/bash # Check if utils.ToString exists and compare implementations rg -n 'func ToString' --type=go -A 5
🧹 Nitpick comments (1)
hooks.go (1)
318-328: Startup message hooks intentionally don’t return errors
These executors use handler types withouterrorreturns (unlike other hooks), so failures can’t be signaled or logged. Consider adding a code comment to document this design choice; if you need error handling here, update the handler signatures to returnerror.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hooks.go(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (Make target:make format).
Run golangci-lint for linting (Make target:make lint).
Rungo vetas part of audit to catch suspicious constructs (Make target:make audit).
Optimize struct field alignment with betteralign (Make target:make betteralign).
Applygopls modernizeto update code patterns (Make target:make modernize).
Files:
hooks.go
🧬 Code graph analysis (1)
hooks.go (2)
color.go (1)
Colors(8-53)app.go (2)
Version(37-37)Map(43-43)
⏰ 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 (2)
hooks.go (2)
21-24: LGTM! Well-designed handler signatures.The pre-startup handler receives a pointer to enable modification of the startup message data (e.g., setting
PreventDefaultor modifying maps), while the post-startup handler receives a value for read-only notification. This design correctly reflects the intended use of each hook.
202-214: LGTM! Thread-safe hook registration.Both
OnPreStartupMessageandOnPostStartupMessagemethods follow the established pattern for hook registration with proper mutex locking, ensuring thread-safe handler registration.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
listen.go (2)
373-391: Simplify pre-startup guard checks
newPreStartupMessageDatanever returns nil; drop redundant nil checks.Apply this diff:
- prevented := preData != nil && preData.PreventDefault + prevented := preData.PreventDefault ... - if preData == nil || disabled || isChild || prevented { + if disabled || isChild || prevented { return }
351-355: Use idiomatic slice clone for child PIDsSimplify the copy;
appendhandles nil/empty slices.Apply this diff:
-var clonedPIDs []int -if len(childPIDs) > 0 { - clonedPIDs = append(clonedPIDs, childPIDs...) -} +clonedPIDs := append([]int(nil), childPIDs...)
🧹 Nitpick comments (1)
listen_test.go (1)
526-531: Avoid Go 1.22-onlyfor range intin tests (compatibility)
for range 10requires Go ≥1.22. If the repo supports older Go, switch to a classic counter loop.Apply this diff in both places:
- childPIDs := make([]int, 0, len(childTemplate)*10) - for range 10 { - childPIDs = append(childPIDs, childTemplate...) - } + childPIDs := make([]int, 0, len(childTemplate)*10) + for i := 0; i < 10; i++ { + childPIDs = append(childPIDs, childTemplate...) + }Please confirm the minimum Go version in go.mod; if it's 1.22+, you can keep the current form.
Also applies to: 560-565
📜 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(6 hunks)hooks_test.go(2 hunks)listen.go(5 hunks)listen_test.go(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/api/hooks.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (Make target:make format).
Run golangci-lint for linting (Make target:make lint).
Rungo vetas part of audit to catch suspicious constructs (Make target:make audit).
Optimize struct field alignment with betteralign (Make target:make betteralign).
Applygopls modernizeto update code patterns (Make target:make modernize).
Files:
hooks_test.golisten.golisten_test.gohooks.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Run the test suite with gotestsum (Make target:make test).
Run benchmarks withgo test(Make target:make benchmark).
Files:
hooks_test.golisten_test.go
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/whats_new.md
🧬 Code graph analysis (4)
hooks_test.go (3)
app.go (4)
New(515-628)Config(113-418)Version(37-37)Map(43-43)listen.go (1)
ListenConfig(42-125)hooks.go (3)
Hooks(36-52)ListenData(61-76)PreStartupMessageData(79-88)
listen.go (5)
app.go (2)
App(68-110)Version(37-37)hooks.go (1)
ListenData(61-76)log/fiberlog.go (1)
Errorf(49-51)prefork.go (1)
IsChild(31-33)color.go (1)
Colors(8-53)
listen_test.go (4)
app.go (2)
New(515-628)Map(43-43)listen.go (1)
ListenConfig(42-125)hooks.go (3)
Hooks(36-52)PreStartupMessageData(79-88)PostStartupMessageData(100-106)prefork.go (1)
IsChild(31-33)
hooks.go (3)
color.go (1)
Colors(8-53)app.go (2)
Version(37-37)Map(43-43)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). (4)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (4)
hooks_test.go (1)
292-345: Solid metadata and hook propagation testCovers ListenData fields, prefork/PIDs, and pre-startup info mutation with proper assertions. Looks good.
listen_test.go (1)
627-660: Pre/Post startup message hooks behavior tests are clearValidates header override, custom entries, suppression, and disabled cases; also checks post flags. Nicely done.
hooks.go (2)
90-98: Pre-startup data cloning is safe and minimalChild PID slice is defensively cloned; good practice for hook isolation.
122-140: Deterministic entry rendering helper is cleanStable ordering and fmt.Sprint conversion make custom info predictable.
| 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.
Restore HeaderSet = true in the example
PreStartupMessageData still relies on HeaderSet to decide whether the custom header should replace the default banner (see hooks_startup.go). Without setting it to true, the override in this snippet will be ignored, so users copying the docs will believe the hook is broken. Please add the flag back.
app.Hooks().OnPreStartupMessage(func(sm *fiber.PreStartupMessageData) error {
sm.Header = "FOOBER " + sm.Version + "\n-------"
+ sm.HeaderSet = true
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
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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.HeaderSet = true | |
| 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 | |
| }) |
🤖 Prompt for AI Agents
In docs/whats_new.md around lines 303 to 308, the example OnPreStartupMessage
handler modifies sm.Header but fails to set sm.HeaderSet = true, so the custom
header will be ignored; update the snippet to set sm.HeaderSet = true (e.g.,
sm.HeaderSet = true) after assigning sm.Header so the PreStartupMessageData will
treat the custom header as intended (see hooks_startup.go for the HeaderSet
behavior).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
listen.go (3)
351-354: Use idiomatic slice clone.Simplify child PID cloning to a one-liner.
- var clonedPIDs []int - if len(childPIDs) > 0 { - clonedPIDs = append(clonedPIDs, childPIDs...) - } + clonedPIDs := append([]int(nil), childPIDs...)
378-391: Remove redundant nil checks; simplify banner suppression logic.
newPreStartupMessageDatanever returns nil, so these checks are unnecessary. Keep logic minimal.- disabled := cfg.DisableStartupMessage - isChild := IsChild() - prevented := preData != nil && preData.PreventDefault + disabled := cfg.DisableStartupMessage + isChild := IsChild() + prevented := preData.PreventDefault … - if preData == nil || disabled || isChild || prevented { + if disabled || isChild || prevented { return }
400-409: Minor: avoid double fmt when printing ASCII art.Optional micro-optimization/readability.
- fmt.Fprintf(out, "%s\n", fmt.Sprintf(figletFiberText, colors.Red+"v"+listenData.Version+colors.Reset)) + fmt.Fprintf(out, figletFiberText+"\n", colors.Red+"v"+listenData.Version+colors.Reset)hooks.go (3)
90-98: Initialize maps to support additive usage.Pre-initialize info maps so handlers can add entries without replacing or risking nil-map assignment.
func newPreStartupMessageData(listenData ListenData) *PreStartupMessageData { clone := listenData if len(listenData.ChildPIDs) > 0 { clone.ChildPIDs = append([]int(nil), listenData.ChildPIDs...) } - return &PreStartupMessageData{ListenData: &clone} + return &PreStartupMessageData{ + ListenData: &clone, + PrimaryInfo: make(Map), + SecondaryInfo: make(Map), + } }
108-120: Use concise slice clone for ChildPIDs.Same optimization as above.
func newPostStartupMessageData(listenData ListenData, disabled, isChild, prevented bool) PostStartupMessageData { clone := listenData - if len(listenData.ChildPIDs) > 0 { - clone.ChildPIDs = append([]int(nil), listenData.ChildPIDs...) - } + clone.ChildPIDs = append([]int(nil), listenData.ChildPIDs...)
60-76: Struct alignment note (optional).Either run betteralign or add an ignore with rationale as public field order prioritizes API clarity over packing.
-type ListenData struct { +type ListenData struct { //betteralign:ignore // Field order prioritizes logical grouping and public API clarity over memory packing.Please run
make betteralignand keep or add the ignore if it proposes a reordering that would harm the public docs layout.
📜 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(6 hunks)hooks_test.go(2 hunks)listen.go(5 hunks)listen_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- listen_test.go
- docs/api/hooks.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (Make target:make format).
Run golangci-lint for linting (Make target:make lint).
Rungo vetas part of audit to catch suspicious constructs (Make target:make audit).
Optimize struct field alignment with betteralign (Make target:make betteralign).
Applygopls modernizeto update code patterns (Make target:make modernize).
Files:
hooks_test.golisten.gohooks.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Run the test suite with gotestsum (Make target:make test).
Run benchmarks withgo test(Make target:make benchmark).
Files:
hooks_test.go
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/whats_new.md
🧬 Code graph analysis (3)
hooks_test.go (3)
app.go (3)
Config(113-418)Version(37-37)Map(43-43)listen.go (1)
ListenConfig(42-125)hooks.go (3)
Hooks(36-52)ListenData(61-76)PreStartupMessageData(79-88)
listen.go (5)
app.go (2)
App(68-110)Version(37-37)hooks.go (1)
ListenData(61-76)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 (2)
Version(37-37)Map(43-43)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: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (4)
hooks_test.go (1)
292-345: Solid coverage for ListenData and startup hooks.Assertions match implementation; good validation of prefork metadata and map propagation via pre-startup hook.
docs/whats_new.md (1)
298-320: Hook example matches API.Signatures and flags (PreventDefault/Disabled/IsChild/Prevented) are correct and consistent with code.
hooks.go (2)
122-140: mapToEntries: good; stable order and type-safe stringification.Sorted keys + fmt.Sprint for values is fine.
318-336: Hook execution mirrors existing patterns.Returning on first error is consistent with other execute* methods.
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: 98445ba | Previous: a167c6f | Ratio |
|---|---|---|---|
Benchmark_Compress/Zstd - B/op |
1 B/op |
0 B/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
|
I suggest removing the use of Map (PrimaryInfo, SecondaryInfo) as the public API and giving the user control over the prepared array of info records. |
|
@Mopsgamer Example? |
Before 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}
return nil
})Afterapp.Hooks().OnPreStartupMessage(func(sm *fiber.PreStartupMessageData) {
sm.Header = "FOOBER " + sm.Version + "\n-------"
for i := range sm.Messages {
if sm.Messages[i].Name == "Process count" {
sm.Messages[i].Value = "\t" + utils.ToString(sm.ProcessCount)
break
}
}
sm.Messages = append(sm.Messages, fiber.StartupMessage{Level: "INFO", "Git hash", "\t\t"+os.Getenv("GIT_HASH")})
// Sort manually
// No errors
}) |
|
@Mopsgamer That's not user friendly at all. We need a better approach at this. |
type StartupMessage struct {
Level string
Priority int
Value string
}
sm.messages map[string]StartupMessagePriorities should update automatically on add, update, and delete. app.Hooks().OnPreStartupMessage(func(sm *fiber.PreStartupMessageData) {
// Prepend
sm.AddInfo("Git hash", os.Getenv("GIT_HASH"), -1)
// Append
sm.AddWarn("Process count", strconv.Itoa(sm.ProcessCount), 99)
realPriority := sm.AddFail("Database", "Connection failed", 5)
// -1 == error == exists
// Move up by 1
sm.SetPriority(sm.GetPriority("Git hash")-1, "Git hash", "Process count")
// Update message value, level, or name
sm.SetValue("Git hash", "new-git-hash-value")
sm.SetLevel("Process count", "WARN")
sm.SetName("Git hash", "Revision")
// Delete a single message
sm.Delete("Database")
// Clear all messages
sm.Delete()
// Set custom formatter using Format property (def. nil)
sm.Format = func(m fiber.StartupMessage) string {
return m.Level + ": " + m.Name + " = " + m.Value
}
})
// Helper method signatures
func (sm *fiber.PreStartupMessageData) AddInfo(name, value string, priority int) int
func (sm *fiber.PreStartupMessageData) AddWarn(name, value string, priority int) int
func (sm *fiber.PreStartupMessageData) AddFail(name, value string, priority int) int
func (sm *fiber.PreStartupMessageData) SetPriority(priority int, names ...string) int
func (sm *fiber.PreStartupMessageData) GetPriority(name string) int
func (sm *fiber.PreStartupMessageData) Delete(names ...string)
func (sm *fiber.PreStartupMessageData) SetValue(name, value string)
func (sm *fiber.PreStartupMessageData) SetLevel(name, level string)
func (sm *fiber.PreStartupMessageData) SetName(oldName, newName string) |
|
Closing this for now, will revisit in the future. The solution is not very UX friendly. |
Summary
This PR enriches the ListenData structure with comprehensive metadata and introduces new hooks for customizing the startup banner message. The changes enable developers to modify or suppress the default Fiber startup message and receive notifications about the message's display status.
Key changes:
Fixes #3800