Add channel settings tab demo#204
Open
nickmisasi wants to merge 8 commits into
Open
Conversation
Register a ChannelSettingsTab pluggable for smoke-testing the new channel settings tab API. The component renders channel info, a dirty-state input, and a tab-switch error banner. Make GetTeams() calls non-fatal in ensureDemoUser, ensureDemoChannels, OnActivate, and OnDeactivate so the plugin can start even when the Teams table has NULL string columns (core server bug). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Contributor
|
Too much diff to scan? Review this PR in Change Stack to start with the highest-impact changes. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces a complete channel settings feature that allows per-channel customization through two UI tabs (schema-driven and custom). The server exposes four HTTP endpoints (GET/POST schema and custom state) with authorization guards. The client module provides typed fetch helpers, and React components render both predefined settings (accent colour, channel info) and a custom tab for channel notes and greeting configuration. ChangesChannel Settings Capability
Sequence Diagram(s)sequenceDiagram
participant UIComponent as UI Component
participant ClientModule as client.ts
participant HTTPHandler as HTTP Handler
participant KVStore as KV Store
UIComponent->>ClientModule: fetchSchemaValues(channelId)
ClientModule->>ClientModule: Build plugin URL
ClientModule->>ClientModule: Get CSRF token via Client4.getOptions
ClientModule->>HTTPHandler: GET /channel_settings/{channel_id}/schema
HTTPHandler->>HTTPHandler: Authorize (requireChannelReader)
HTTPHandler->>KVStore: Get schema data
KVStore-->>HTTPHandler: Channel data map
HTTPHandler-->>ClientModule: 200 JSON response
ClientModule-->>UIComponent: Promise<ChannelSettingsValues>
UIComponent->>ClientModule: saveSchemaValues(channelId, newValues)
ClientModule->>HTTPHandler: POST /channel_settings/{channel_id}/schema
HTTPHandler->>HTTPHandler: Authorize (requireChannelManager)
HTTPHandler->>KVStore: Set schema data
KVStore-->>HTTPHandler: Success
HTTPHandler-->>ClientModule: 200 JSON response
ClientModule-->>UIComponent: Promise<ChannelSettingsValues>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
webapp/src/components/channel_settings_smoke_test.jsx (1)
17-24: Consider defensive access for channel properties.While
channelis marked as required in PropTypes, accessing nested properties without defensive checks could cause runtime errors if the channel object has an unexpected shape. For a smoke test component this is likely acceptable, but consider using optional chaining for robustness.♻️ Optional defensive access
<div style={{marginTop: '12px'}}> - <strong>{'Display Name: '}</strong>{channel.display_name} + <strong>{'Display Name: '}</strong>{channel?.display_name} </div> <div style={{marginTop: '4px'}}> - <strong>{'Channel Name: '}</strong>{channel.name} + <strong>{'Channel Name: '}</strong>{channel?.name} </div> <div style={{marginTop: '4px'}}> - <strong>{'Channel ID: '}</strong>{channel.id} + <strong>{'Channel ID: '}</strong>{channel?.id} </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/components/channel_settings_smoke_test.jsx` around lines 17 - 24, Accessing channel.display_name, channel.name and channel.id without defensive checks can throw if channel has an unexpected shape; update the JSX to use optional chaining and sensible fallbacks (e.g. channel?.display_name ?? '' , channel?.name ?? '' , channel?.id ?? '') so missing properties render safely. Locate the render usage of channel.display_name / channel.name / channel.id in the channel settings smoke test component and replace direct property access with the optional-chaining + fallback pattern to make the component robust.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@webapp/src/components/channel_settings_smoke_test.jsx`:
- Around line 17-24: Accessing channel.display_name, channel.name and channel.id
without defensive checks can throw if channel has an unexpected shape; update
the JSX to use optional chaining and sensible fallbacks (e.g.
channel?.display_name ?? '' , channel?.name ?? '' , channel?.id ?? '') so
missing properties render safely. Locate the render usage of
channel.display_name / channel.name / channel.id in the channel settings smoke
test component and replace direct property access with the optional-chaining +
fallback pattern to make the component robust.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ff8b33f-4acf-4e51-a81e-8586416deed2
📒 Files selected for processing (4)
server/activate_hooks.goserver/configuration.gowebapp/src/components/channel_settings_smoke_test.jsxwebapp/src/plugin.jsx
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
nickmisasi
changed the title
hanzei
reviewed
Mar 19, 2026
hanzei
left a comment
hanzei
left a comment
Contributor
There was a problem hiding this comment.
I'm not sure I agree with the non-fatal changes
| msg := fmt.Sprintf("OnActivate: %s", manifest.Id) | ||
| if err := p.postPluginMessage(team.Id, msg); err != nil { | ||
| return errors.Wrap(err, "failed to post OnActivate message") | ||
| p.API.LogWarn("Failed to query teams OnActivate, skipping activation messages", "error", err.Error()) |
Contributor
There was a problem hiding this comment.
It's strange that GetTeams fails. I wonder if we should still fail loudly as that makes things easier for QA to debug.
Contributor
There was a problem hiding this comment.
I would love to see a typescript file. not a blocker
- Register save/reset via registerSaveBarHandlers; remove inline tab-switch banner - Set icon to fa fa-plug to match other demo plugin surfaces (MainMenu, channel header, etc.) Made-with: Cursor
Made-with: Cursor
* feat: rewrite channel settings to schema-based webapp API
Migrate the demo plugin from the old custom-only channel settings API
(registerSaveBarHandlers) to the new schema-based registry API so the
plugin serves as a reference exemplar for both registration styles.
Register two channel settings tabs:
- A declarative schema tab ("Demo Channel Settings") with a Posting
section (two radio settings + a custom accent-color setting) and a
custom Channel info section. shouldRender hides it in DM/GM, the host
owns the save bar, and persistence flows through onSave/loadValues.
- A fully custom tab ("Demo Advanced (Custom)") where the plugin renders
the entire body, registers its own save/reset handlers, and reports
dirty state via setUnsaved.
Persistence is backed by new plugin HTTP endpoints over the KV store
(GET/POST /channel_settings/{channel_id}/schema and /custom) guarded by
read (PermissionReadChannel) and write (Manage{Public,Private}
ChannelProperties) authorization. Removes the old smoke-test component.
Co-authored-by: Cursor <[email protected]>
* fix: make custom channel settings textarea theme-aware
Theme the "Channel note" textarea via Mattermost CSS variables so it
renders correctly on any theme, and add a visible translatable
placeholder (injectIntl) so it's readable on dark themes.
Co-authored-by: Cursor <[email protected]>
---------
Co-authored-by: Cursor <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
webapp/src/channel_settings/client.ts (1)
22-28: 💤 Low valueConsider error response handling.
The
request<T>function uses a type assertion (as Promise<T>) without validating the response shape. If the server returns an error with an unexpected JSON structure, the typed result could cause runtime issues in consuming code.For a production plugin, consider validating critical fields or wrapping the response:
async function request<T>(url: string, options: Options): Promise<T> { const res = await fetch(url, Client4.getOptions(options)); if (!res.ok) { // Try to extract error message from response const errorText = await res.text().catch(() => 'Unknown error'); throw new Error(`channel settings request failed: ${res.status} - ${errorText}`); } return res.json() as Promise<T>; }However, for a demo plugin this is acceptable as-is.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/channel_settings/client.ts` around lines 22 - 28, The request<T> function currently asserts the JSON response shape without handling error bodies; update request<T> to attempt to read error details when res.ok is false (e.g., call res.text() or parse res.json() safely) and include that detail in the thrown Error, and optionally validate or guard the successful res.json() before casting to T so callers don't get a misleading type assertion; modify the function named request<T> in client.ts to extract and include response error text/JSON in the error message and add a basic runtime check or explicit parsing before returning the typed result.server/channel_settings_api.go (1)
102-107: ⚡ Quick winMove
defer r.Body.Close()before decoding.The
deferstatement should be placed immediately after you begin using the body, not afterDecode. While this works in practice becauseDecodeconsumes the body, the conventional pattern is:var values map[string]string defer r.Body.Close() if err := json.NewDecoder(r.Body).Decode(&values); err != nil {This makes the resource cleanup intent clearer and matches idiomatic Go.
📋 Proposed fix
func (p *Plugin) handleSaveChannelSettingsSchema(w http.ResponseWriter, r *http.Request) { channelID := mux.Vars(r)["channel_id"] if _, ok := p.requireChannelManager(w, r, channelID); !ok { return } var values map[string]string + defer r.Body.Close() if err := json.NewDecoder(r.Body).Decode(&values); err != nil { http.Error(w, "Invalid request body", http.StatusBadRequest) return } - defer r.Body.Close()Apply the same pattern to
handleSaveChannelSettingsCustomat line 145.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/channel_settings_api.go` around lines 102 - 107, Move the defer r.Body.Close() to immediately after the request body is first used in the handlers to follow idiomatic Go: in handleSaveChannelSettings (the block that currently does json.NewDecoder(r.Body).Decode(&values)) place defer r.Body.Close() before calling Decode so the body is scheduled to be closed as soon as it’s referenced; apply the same change in handleSaveChannelSettingsCustom to ensure both handlers defer closing r.Body before decoding the JSON.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/channel_settings_api.go`:
- Around line 84-88: In handleGetChannelSettingsSchema: stop pre-initializing
values to an empty map; declare it as var values map[string]string so
p.client.KV.Get(csSchemaKey(channelID), &values) can return nil without creating
an empty map, then after the GET check err (log and 500 on error) and explicitly
handle values == nil as the missing-key case (e.g., respond with 404 or an
empty-schema response as your API requires) instead of relying on the prior
redundant empty-map check; reference p.client.KV.Get, csSchemaKey, and the
values variable when making this change.
---
Nitpick comments:
In `@server/channel_settings_api.go`:
- Around line 102-107: Move the defer r.Body.Close() to immediately after the
request body is first used in the handlers to follow idiomatic Go: in
handleSaveChannelSettings (the block that currently does
json.NewDecoder(r.Body).Decode(&values)) place defer r.Body.Close() before
calling Decode so the body is scheduled to be closed as soon as it’s referenced;
apply the same change in handleSaveChannelSettingsCustom to ensure both handlers
defer closing r.Body before decoding the JSON.
In `@webapp/src/channel_settings/client.ts`:
- Around line 22-28: The request<T> function currently asserts the JSON response
shape without handling error bodies; update request<T> to attempt to read error
details when res.ok is false (e.g., call res.text() or parse res.json() safely)
and include that detail in the thrown Error, and optionally validate or guard
the successful res.json() before casting to T so callers don't get a misleading
type assertion; modify the function named request<T> in client.ts to extract and
include response error text/JSON in the error message and add a basic runtime
check or explicit parsing before returning the typed result.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09d28ce8-462b-4d78-bdda-4b1181a6160c
⛔ Files ignored due to path filters (1)
webapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
go.modserver/channel_settings_api.goserver/channel_settings_api_test.goserver/http_hooks.gowebapp/i18n/en.jsonwebapp/src/channel_settings/accent_color_setting.tsxwebapp/src/channel_settings/channel_info_section.tsxwebapp/src/channel_settings/client.tswebapp/src/channel_settings/custom_tab.tsxwebapp/src/channel_settings/register.tswebapp/src/channel_settings/schema_tab.tsxwebapp/src/channel_settings/types.tswebapp/src/plugin.jsx
✅ Files skipped from review due to trivial changes (1)
- webapp/i18n/en.json
Address @hanzei's PR #204 review feedback: GetTeams failing during OnActivate should fail loudly (return the error and abort activation) rather than LogWarn-and-continue, so QA can debug activation problems. Scoped to OnActivate per the review comment; OnDeactivate and the ensureDemo* helpers keep their resilient behavior. Co-authored-by: Cursor <[email protected]>
…uggable Co-authored-by: Cursor <[email protected]> # Conflicts: # go.mod
… tab The host no longer injects theme/webSocketClient into custom channel settings tab bodies, so the reference implementation reads colors from the Mattermost theme CSS variables instead of the theme prop and drops the now-unneeded component cast. Co-authored-by: nick.misasi <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ChannelSettingsTabpluggable to smoke-test the new channel settings tab plugin APIChannelSettingsSmokeTestcomponent that renders channel info, a dirty-state input, and a tab-switch error bannerGetTeams()calls non-fatal inensureDemoUser,ensureDemoChannels,OnActivate, andOnDeactivateso the plugin can start even when the Teams table has NULL string columns (core server bug whereCompanyNameis NULL)Needs Support pluggable channel settings tabs mattermost#35591
Test plan
GetTeams()returns an error (check server logs for warn-level messages instead of crashes)🤖 Generated with Claude Code