🐛 bug: Reload mounted view engines in ReloadViews#4031
Conversation
### Motivation - Ensure `ReloadViews()` reloads view engines for mounted sub-apps so on-disk template changes are picked up when called on the parent application. - Prevent panics by skipping nil and typed-nil view engines during reload. - Fix lint feedback by returning mount helpers directly from `Use` helpers for `App` and `Group`. ### Description - Updated `App.ReloadViews()` to iterate `app.mountFields.appList` when available, call `Load()` on each mounted app's `config.Views`, skip `nil`/typed-nil engines, and return `ErrNoViewEngineConfigured` only if nothing was reloaded. - Preserved the typed-nil guard by checking `reflect.ValueOf(...).Kind() == reflect.Pointer && IsNil()` before calling `Load()`. - Changed `Use` in `app.go` and `group.go` to return the result of `mount(...)` directly to satisfy lint expectations without changing runtime behavior. - Added `fileView` test helper and a unit test `Test_App_ReloadViews_MountedViews` in `app_test.go` that mounts a sub-app with its own `Views`, updates an on-disk template, calls `ReloadViews()` on the parent, and asserts the sub-app engine reloaded. ### Testing - Ran `make audit` (failed due to `govulncheck` findings in the Go 1.25.1 standard library; this is an environment/tooling scanner result, not a functional test failure). - Ran `make generate`, `make betteralign`, `make modernize`, and `make format`, all completed successfully. - Ran `make lint`, which completed with no reported issues after the changes. - Ran `make test`, which executed the full test suite including the new `Test_App_ReloadViews_MountedViews`; the test suite completed successfully (all tests passed, some tests skipped as expected). - The new unit test `Test_App_ReloadViews_MountedViews` passes and verifies that a mounted sub-app's view engine reloads when the parent `ReloadViews()` is invoked.
|
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. WalkthroughReloadViews() now reloads view engines for all mounted apps (including sub-apps) by iterating mountFields.appList; mounting/Use() returns mount(...) result when attaching a sub-app. Tests add a file-backed view helper and verify reload propagation, error handling, and multi-app reloads. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant App
participant MountedApp
participant ViewEngine
Caller->>App: ReloadViews()
activate App
App->>App: collect mountFields.appList
loop for each mounted target
App->>MountedApp: validate target (non-nil, has Views)
alt valid target
MountedApp->>ViewEngine: Views.Load()
ViewEngine-->>MountedApp: success / error
MountedApp-->>App: mark reloaded or propagate error
else skip
end
end
App-->>Caller: return nil | ErrNoViewEngineConfigured | wrapped error
deactivate App
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Summary of ChangesHello @gaby, 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 addresses issues related to view engine management, particularly when dealing with mounted sub-applications. The primary goal is to ensure that template changes are consistently reflected across all parts of a Fiber application, including those mounted as sub-apps, by enhancing the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code 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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4031 +/- ##
=======================================
Coverage 91.21% 91.22%
=======================================
Files 119 119
Lines 11101 11111 +10
=======================================
+ Hits 10126 10136 +10
Misses 618 618
Partials 357 357
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.
Code Review
This pull request correctly extends ReloadViews to also reload view engines for mounted sub-applications, which is a great bug fix. The added tests are thorough and validate the new behavior effectively. The changes to the Use methods in App and Group are good stylistic improvements.
I've found one potential concurrency issue. The new ReloadViews function reads from app.mountFields.appList with a lock, but the mount functions that write to this map do not use a lock, which can lead to a data race. I've left a detailed comment on how to address this.
There was a problem hiding this comment.
Pull request overview
This PR enhances the ReloadViews() method to reload view engines for mounted sub-applications, addressing issue #3875 which requested the ability to reload templates from disk without restarting the application. The changes ensure that when ReloadViews() is called on a parent app, all mounted sub-apps with configured view engines are also reloaded.
Changes:
- Extended
ReloadViews()to iterate through mounted sub-apps and reload their view engines - Added nil and typed-nil safety checks to prevent panics when view engines are nil
- Refactored
Use()methods in bothAppandGroupto return mount results directly for cleaner code - Added test infrastructure (
fileViewhelper) and comprehensive test case for mounted view reloading
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app.go | Enhanced ReloadViews() to handle mounted apps and refactored Use() for direct returns |
| group.go | Refactored Use() to return mount result directly, consistent with app.go changes |
| app_test.go | Added fileView test helper and Test_App_ReloadViews_MountedViews test case |
|
Fixing AI review |
### Motivation - Ensure `ReloadViews()` reloads view engines for mounted sub-apps so on-disk template changes are picked up when invoked on the parent application. - Prevent panics by skipping `nil` and typed-nil view engine values when reloading. - Keep lint expectations by returning mount helpers directly from `Use` helpers without changing runtime behavior. ### Description - Updated `App.ReloadViews()` to iterate mounted apps (`app.mountFields.appList` when present) and call `Load()` on each mounted app's `config.Views`, skipping `nil` and typed-nil engines and returning `ErrNoViewEngineConfigured` only if nothing was reloaded. - Preserved the typed-nil guard by checking `reflect.ValueOf(...).Kind() == reflect.Pointer && IsNil()` before calling `Load()` and wrap load errors with a descriptive message. - Changed `App.Use` and `Group.Use` to return the result of `mount(...)` directly to satisfy lint expectations without changing runtime behavior. - Moved the `fileView` test helper to the top of `app_test.go` and added `Test_App_ReloadViews_MountedViews` to verify that a mounted sub-app's view engine is reloaded when the parent `ReloadViews()` is invoked. ### Testing - Ran `make audit` which failed due to `govulncheck` reporting vulnerabilities in the Go 1.25.1 standard library (tooling scanner result, not a functional test failure). - Ran `make generate` which completed successfully. - Ran `make betteralign` which completed successfully. - Ran `make modernize` which completed successfully. - Ran `make format` which completed successfully. - Ran `make lint` which completed successfully with no reported issues after the changes. - Ran `make test` which executed the full test suite and completed successfully, e.g. `DONE 2603 tests, 1 skipped` (all non-skipped tests passed).
Motivation
ReloadViews()reloads view engines for mounted sub-apps so on-disk template changes are picked up when invoked on the parent application.niland typed-nil view engine values when reloading.Usehelpers without changing runtime behavior.Description
App.ReloadViews()to iterate mounted apps (app.mountFields.appListwhen present) and callLoad()on each mounted app'sconfig.Views, skippingniland typed-nil engines and returningErrNoViewEngineConfiguredonly if nothing was reloaded.reflect.ValueOf(...).Kind() == reflect.Pointer && IsNil()before callingLoad()and wrap load errors with a descriptive message.App.UseandGroup.Useto return the result ofmount(...)directly to satisfy lint expectations without changing runtime behavior.fileViewtest helper to the top ofapp_test.goand addedTest_App_ReloadViews_MountedViewsto verify that a mounted sub-app's view engine is reloaded when the parentReloadViews()is invoked.Fixes #3875