Create modular ui read real time config in system console#205
Create modular ui read real time config in system console#205christopherfickess wants to merge 4 commits intomasterfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 25 minutes and 14 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughgetConfiguration() in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/configuration.go (1)
107-119: Unify configuration source of truth after this change.Lines 107-119 now bypass
p.configuration, butOnConfigurationChangestill updates it viasetConfiguration. This split strategy is confusing; either use cached state as deliberate fallback/source, or remove the cache/lock path and update the surrounding docs accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/configuration.go` around lines 107 - 119, The code now reads live config via getConfiguration() (calling p.API.LoadPluginConfiguration) but OnConfigurationChange/setConfiguration still update the cached p.configuration, creating two sources of truth; pick one approach and make it consistent: either (A) remove the cached p.configuration and any p.configurationLock and update OnConfigurationChange to stop calling setConfiguration and update docs/comments to state all reads use getConfiguration(), or (B) revert getConfiguration() to return the cached p.configuration (protected by p.configurationLock) and ensure LoadPluginConfiguration is only used inside setConfiguration (called by OnConfigurationChange) so all readers use the cache; update or remove the related comments accordingly and keep references to getConfiguration, setConfiguration, OnConfigurationChange, and p.configuration consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/configuration.go`:
- Around line 113-116: When p.API.LoadPluginConfiguration(config) fails, do not
return a zero-value &configuration{}; instead acquire the read lock for the
cached configuration (e.g., p.configLock.RLock()), read and return the
last-known-good config (e.g., p.cachedConfig or similar) so handlers relying on
IsValid() get the prior valid settings, then release the lock; also keep the
p.API.LogError call but add context that you're falling back to the cached
config.
---
Nitpick comments:
In `@server/configuration.go`:
- Around line 107-119: The code now reads live config via getConfiguration()
(calling p.API.LoadPluginConfiguration) but
OnConfigurationChange/setConfiguration still update the cached p.configuration,
creating two sources of truth; pick one approach and make it consistent: either
(A) remove the cached p.configuration and any p.configurationLock and update
OnConfigurationChange to stop calling setConfiguration and update docs/comments
to state all reads use getConfiguration(), or (B) revert getConfiguration() to
return the cached p.configuration (protected by p.configurationLock) and ensure
LoadPluginConfiguration is only used inside setConfiguration (called by
OnConfigurationChange) so all readers use the cache; update or remove the
related comments accordingly and keep references to getConfiguration,
setConfiguration, OnConfigurationChange, and p.configuration consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 702e4418-e19e-4fb5-a790-f3664109a309
📒 Files selected for processing (1)
server/configuration.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/command_test.go (1)
35-40: Consider extracting sharedLoadPluginConfigurationmock setup into a test helper.Line [35] to Line [40] repeats the same wiring used in multiple command test files. A helper would reduce maintenance drift.
♻️ Suggested helper extraction
+// e.g. in server/test_helpers_test.go +func mockPluginConfigurationLoad(api *plugintest.API, plugin *Plugin) { + api.On("LoadPluginConfiguration", mock.Anything).Return(nil).Run(func(args mock.Arguments) { + cfg := args.Get(0).(*configuration) + if plugin.configuration != nil { + *cfg = *plugin.configuration + } + }) +}- api.On("LoadPluginConfiguration", mock.Anything).Return(nil).Run(func(args mock.Arguments) { - cfg := args.Get(0).(*configuration) - if plugin.configuration != nil { - *cfg = *plugin.configuration - } - }) + mockPluginConfigurationLoad(api, &plugin)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/command_test.go` around lines 35 - 40, Extract the repeated mock wiring for LoadPluginConfiguration into a test helper to avoid duplication: create a helper function (e.g., mockLoadPluginConfiguration or setupLoadPluginConfigurationMock) in the test package that accepts the mock API object and the plugin (or its configuration), and inside it perform api.On("LoadPluginConfiguration", mock.Anything).Return(nil).Run(func(args mock.Arguments) { cfg := args.Get(0).(*configuration); if plugin.configuration != nil { *cfg = *plugin.configuration } }); then replace the repeated block in server/command_test.go and other tests with a single call to this helper so all tests share the same setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/command_test.go`:
- Around line 35-40: Extract the repeated mock wiring for
LoadPluginConfiguration into a test helper to avoid duplication: create a helper
function (e.g., mockLoadPluginConfiguration or setupLoadPluginConfigurationMock)
in the test package that accepts the mock API object and the plugin (or its
configuration), and inside it perform api.On("LoadPluginConfiguration",
mock.Anything).Return(nil).Run(func(args mock.Arguments) { cfg :=
args.Get(0).(*configuration); if plugin.configuration != nil { *cfg =
*plugin.configuration } }); then replace the repeated block in
server/command_test.go and other tests with a single call to this helper so all
tests share the same setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6935b723-b839-4bc5-8d40-44c4b56a007c
📒 Files selected for processing (7)
server/command_attach_message_test.goserver/command_copy_thread_test.goserver/command_list_channels_test.goserver/command_list_messages_test.goserver/command_merge_thead_test.goserver/command_move_thread_test.goserver/command_test.go
…st works as expected
…st works as expected
1 participant
Change Impact: 🟡 Medium
Reasoning: The plugin switches from returning a cached, lock-protected configuration to loading the plugin configuration live from Mattermost on every getConfiguration() call, changing the consistency and performance characteristics across command handlers and other internal paths.
Regression Risk: Moderate. The change affects many call sites that previously relied on the cached, immutable configuration (commands, authorization checks, helpers). Tests were updated to mock LoadPluginConfiguration, but the runtime behavior introduces potential race/consistency and performance regressions not fully covered by existing tests.
QA Recommendation: Perform focused manual QA: validate real-time config updates are applied without restart, exercise concurrent command execution to detect race conditions, and measure any performance impact from frequent LoadPluginConfiguration() calls. Skipping manual QA is not recommended given configuration touches critical plugin behavior.
Generated by CodeRabbitAI