feat consolidate surface settings #3916
Conversation
- Behavior is similar to the layout on the Connections page: - When nothing is selected the right-panel shows the settings and instance table -- making it easy to disable/enable an integration (and to confirm its state). - Click on any configured surface/group brings up the edit panel as usual, but with the "X" close-icon always visible. - Click on the selected surface or the "X" icon closes the edit panel/reveals the settings - Click on an item in the instances table (right panel) brings up the instance configuration panel in that panel (with the "X" close-icon, of course). - Click on "Add Integration" above the instances table to add a new instance (instead of having to navigate to the other page) The current implementation allows the Configured Surfaces page to coexist with the Integrations page, but that page is no longer necessary. Removing the Integrations page could simplify the conditional coding included here. Notes: - (The inspector in Chrome insists I use thead/tbody or else it emits dozens of DOM errors.) - For the most part this PR repurposes existing code, with some changes to visibility, rather than adding new Components. Formatting (all of which should be taken as suggestions) - Put the integrations table in a table-looking box, make the instructions more consise, fiddle with spacing. - Rearrange switches and improve formatting slightly. - In particular "Enable Elgato software Plugin" is now grouped with the instance table, since it serves a similar function (enabling a class of inputs).
Setting width=100% for the first column in UserConfigSwitchRow squeezes text and numeric entries in tables that include switches and text/number input. This commit fixes the affected settings pages.
When the window is too narrow, a new button is presented to show the settings panel.
This avoids confusion, especially when the panel is opened automatically upon addition of an instance.
- optional `tooltip` property for `UserConfigHeadingRow` - shows the help circle at the right of the heading row.
Add a reusable, style-able panel-close button - Make it very visible, so user see how to "remove" a panel (the plain X is fine one you see it but...) - Generalize the button so it can provide a consistent look across pages - Give it a CSS class so we can fiddle with details - Apply it to the four closeable Surface panels Add a context-help button with similar styling to the panel-close button. It allows both hover help and going to a user-guide page or running a custom function.
In the current beta (not just this PR), the configured surfaces list grows when window-height is greater than a threshold. But this only happens when no row is selected, resulting in the row height changing when you select an item. The cause is partly because the parent container `CCol` just inside `ConfiguredSuracesPage` acquiring class `d-xl-block`, which overrides flex behavior. Removing `flex-grow: 1` from `.scrollable-container` in _sticky-tables.scss fixes it by always suppressing table-height growth. Personally, I don't see any benefit in stretching the row height. No other page/panel appears to be affected by this change.
📝 WalkthroughWalkthroughIntroduces reusable CloseButton and ContextHelpButton components, adds a ConfiguredSurfaceContext and new configured/integration routes, updates panels and lists to use dynamic routing and new props (e.g., isSubpanel, toDir), and adjusts layout/styles for responsive settings UIs. Changes
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🧹 Nitpick comments (5)
webui/src/Surfaces/Instances/SurfaceInstanceEdit/SurfaceInstanceEditPanel.tsx (1)
70-77: Smart dynamic navigation - nice refactor!Great idea deriving the homepage from the current pathname rather than hardcoding. This makes the component more reusable across different route structures.
One small thought: the
pathname.split('/', 3).join('/')approach works perfectly for paths like/surfaces/configured/..., but if this component ever ends up on a shorter path, it might produce unexpected results. Given the route structure though, this seems unlikely to be an issue in practice.If you wanted extra safety, you could add a fallback:
💡 Optional defensive fallback
- const homepage = pathname.split('/', 3).join('/') // first two elements of the path ex: /surfaces/configured + const homepage = pathname.split('/', 3).join('/') || '/surfaces/configured' // fallback if path is unexpectedly shortwebui/src/Surfaces/ConfiguredSurfacesContext.tsx (1)
8-18: Nice context setup! One small note on the validation logic.Thanks for adding the lint comment explaining the file structure - that's helpful context!
One thing to note: since
createContexton line 6 provides a default value, theif (!context)check on lines 10-12 will never trigger - the context will always be at least the default object. This check would only be useful if you passednullorundefinedas the default.The setShowSettings validation on lines 13-17 is more useful since it could catch malformed provider values, though with TypeScript the type system should help prevent that.
If you'd like to catch "used outside provider" scenarios, you could use
nullas the default:💡 Optional pattern for detecting missing provider
-export const ConfiguredSurfaceContext = createContext({ setShowSettings: (_a: boolean) => {} }) +export const ConfiguredSurfaceContext = createContext<{ setShowSettings: (a: boolean) => void } | null>(null) export const useConfiguredSurfaceContext = (): booleanFn => { const context = useContext(ConfiguredSurfaceContext) if (!context) { throw new Error('useConfiguredSurfaceContext must be used within a ConfiguredSurfaceContext provider') } - if (!context.setShowSettings || typeof context.setShowSettings !== 'function') { - throw new Error( - 'ConfiguredSurfaceContext provider must be an object with a setShowSettings property set to a function' - ) - } return context.setShowSettings }Either approach works - the current code is perfectly functional, this is just a pattern suggestion for stricter provider enforcement if you want it!
webui/src/UserConfig/Components/UserConfigHeadingRow.tsx (1)
18-24: Consider reusingContextHelpButtonfor consistency (optional)This works well! Just a small thought: the project now has a
ContextHelpButtoninCommon.tsxthat also wrapsFontAwesomeIcon+InlineHelp. If you wanted to unify the help icon styling across the app, you could potentially use it here too (without theuserGuideprop, it would just show the hover text).That said, this simpler inline approach is totally fine if the styling needs differ here. No action needed if you prefer keeping it as-is!
webui/src/Surfaces/Instances/AddSurfaceInstancePanel.tsx (1)
63-72: Path computation works but is a bit fragileI appreciate the honest comment about this not being pretty! 😄 The logic works for the current route structure, but a few edge cases to keep in mind:
pathname.split('/', 3).join('/')assumes at least 2 path segments existreplace('add', '$instanceId')does a simple string replace - ifaddever appears elsewhere in a path segment, this could misbehaveFor now this seems fine given the controlled route structure. If you wanted to make it more robust later, you could use
pathname.lastIndexOf('/')to safely extract the parent path, or leverage TanStack Router's path building utilities.No immediate action needed - just something to revisit if the routing gets more complex!
webui/src/routes/app/surfaces/configured/index.tsx (1)
4-4: Consider removing commented-out importThis commented-out import might be a leftover from refactoring. If it's no longer needed, feel free to remove it when you get a chance to keep the file tidy. No rush on this one! 🧹
| useComputed(() => { | ||
| if (!surfaceInstances.instances.has(instanceId)) { | ||
| void navigate({ to: '/surfaces/configured' }) | ||
| } | ||
| }, [navigate, surfaceInstances, instanceId]) | ||
|
|
||
| return <SurfaceInstanceEditPanel key={instanceId} instanceId={instanceId} /> |
There was a problem hiding this comment.
Navigation during render may cause issues
The validation logic looks correct, but calling navigate() inside useComputed (which runs during render) could lead to React warnings or unexpected behavior. The component also continues to render SurfaceInstanceEditPanel even when the instance is invalid and navigation is triggered.
A safer pattern would be to either:
- Return early with
nullwhen the instance is invalid, or - Move the navigation to a
useEffect
💡 Suggested approach
function ModuleConfigComponent() {
const { instanceId } = Route.useParams()
const { surfaceInstances } = useContext(RootAppStoreContext)
const navigate = useNavigate()
+ const instanceExists = surfaceInstances.instances.has(instanceId)
- // Ensure the selected instance is valid
- useComputed(() => {
- if (!surfaceInstances.instances.has(instanceId)) {
+ // Redirect if instance doesn't exist
+ React.useEffect(() => {
+ if (!instanceExists) {
void navigate({ to: '/surfaces/configured' })
}
- }, [navigate, surfaceInstances, instanceId])
+ }, [navigate, instanceExists])
+
+ if (!instanceExists) {
+ return null
+ }
return <SurfaceInstanceEditPanel key={instanceId} instanceId={instanceId} />
}| <div className="connection-group-actions mb-2 mx-1"> | ||
| <CButtonGroup> | ||
| <CButton | ||
| color="primary" | ||
| size="sm" | ||
| className="d-xl-none" | ||
| onClick={() => void navigate({ to: '/surfaces/integrations/add' })} | ||
| className={toDir ? 'undefined' : 'd-xl-none'} | ||
| onClick={() => void navigate({ to: `/surfaces/${toDir ?? 'integrations'}/add` })} | ||
| > |
There was a problem hiding this comment.
Avoid literal "undefined" class on the Add button.
This currently injects a real CSS class named undefined when toDir is set.
🛠️ Proposed fix
- className={toDir ? 'undefined' : 'd-xl-none'}
+ className={toDir ? undefined : 'd-xl-none'}📝 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.
| <div className="connection-group-actions mb-2 mx-1"> | |
| <CButtonGroup> | |
| <CButton | |
| color="primary" | |
| size="sm" | |
| className="d-xl-none" | |
| onClick={() => void navigate({ to: '/surfaces/integrations/add' })} | |
| className={toDir ? 'undefined' : 'd-xl-none'} | |
| onClick={() => void navigate({ to: `/surfaces/${toDir ?? 'integrations'}/add` })} | |
| > | |
| <div className="connection-group-actions mb-2 mx-1"> | |
| <CButtonGroup> | |
| <CButton | |
| color="primary" | |
| size="sm" | |
| className={toDir ? undefined : 'd-xl-none'} | |
| onClick={() => void navigate({ to: `/surfaces/${toDir ?? 'integrations'}/add` })} | |
| > |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
webui/src/routes/app/surfaces/configured/index.tsx (3)
4-4: Consider removing the commented-out import.This commented import for
SurfacesConfigappears to be leftover from refactoring. Feel free to remove it when you get a chance to keep things tidy! 🧹
67-77: Prefer CSS for bold styling instead of wrapping the table in<b>.Wrapping a
<table>inside a<b>tag is semantically unusual -<b>is intended for inline phrasing content. While it might render as expected in most browsers, using CSS would be cleaner and more predictable.✨ Suggested approach
- {userConfigProps && ( - <b> - <table className="table table-responsive-sm table-settings ms-3"> - <tbody> - <UserConfigSwitchRow - userConfig={userConfigProps} - label="Enable Elgato software Plugin support" - field="elgato_plugin_enable" - /> - </tbody> - </table> - </b> - )} + {userConfigProps && ( + <table className="table table-responsive-sm table-settings ms-3" style={{ fontWeight: 'bold' }}> + <tbody> + <UserConfigSwitchRow + userConfig={userConfigProps} + label="Enable Elgato software Plugin support" + field="elgato_plugin_enable" + /> + </tbody> + </table> + )}Alternatively, if you'd like to keep styles in CSS, you could add a class for this.
24-29: Optional: Consider extracting repeated inline styles to CSS classes.There are several inline styles throughout the component (padding, margins, borders). While they work fine, extracting them to CSS classes could make future tweaks easier. Totally optional and can be a follow-up if you prefer - no blocker here!
|
|
||
| function SettingsPanelTitleBar() { | ||
| const setPanelVisibility = useConfiguredSurfaceContext() | ||
| // note that the close button is currently hidden |
There was a problem hiding this comment.
The comment might be slightly misleading.
The comment says "close button is currently hidden," but looking at line 110, the button uses visibilityClass="d-xl-none" which means it's actually visible on screens smaller than XL (and hidden only on XL+). If this responsive behavior is intentional, perhaps updating the comment would help future readers understand the design decision?
📝 Suggested comment update
- // note that the close button is currently hidden
+ // note: close button is hidden on XL+ screens (settings panel is always visible there)📝 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.
| // note that the close button is currently hidden | |
| // note: close button is hidden on XL+ screens (settings panel is always visible there) |
|
(Note: I'll wait on comments/approval before dancing with the Rabbit.) |
1 participant
Consolidate settings and integrations with the configured-surfaces page
Previously, the information needed to diagnose surface connection problems was spread across two pages: configured surfaces and settings/surfaces. In 4.3, the with the introduction of the Integrations page, information is spread across three pages. This PR rearranges and consolidates all of this information into a single page, taking advantage of the blank default right-panel on the configured-surfaces page.
(NB: This PR is the continuation of a conversation in PR #3703 that we agreed to defer until after that one was merged, so a purpose of this PR is to give a concrete visualization of my ideas discussed there.)
There's quite a bit of detail here. It may be best to start by watching the video, below.
Features:
Companion.Surface.Demo.reduced.mp4
General Notes:
thead/tbodyto tables or else it emits dozens of DOM errors.)Formatting notes (all of which should be taken as suggestions):
The best way to think about the benefits of this PR is in terms of workflows:
A. Adding new integrations will always be rare relative to configuring/diagnosing surfaces, so for the most part the "root" right-panel is giving you instant enable/disabled status, as well as showing when it's time to update an instance all in the one page. Consider when a surface isn't showing up: is it disabled? is the integration-instance loaded? is the integration disabled? does it need to be updated? All of this information is now immediately seen in one page.
B. Here's how I envision a typical add-integration workflow: 1. plug in your surface 2. nothing shows up in the left panel. 3. look to the right, is the module enabled? 4. oh, it's not installed yet! click on "Add Surface Integration" and add it. 5. now everything's working and you didn't have to find or visit other pages: all the information you need is right there (and taking up what used to be blank space).
Notice that the interaction starts by looking at the left panel and is all done with the goal of getting something to show up in the left panel, i.e., it starts and ends with the configured-surfaces list (without having to flip back and forth between pages), so the new right panel fits naturally on this page.
C. An alternate add-integration workflow would be the person "knowing" that they need to add a new brand of surface and now that they've seen the "Add Surface Integrations" button (and list of installed integrations) every time they were on the surfaces page, they know exactly where to go.. (so maybe an extra click or maybe saves four clicks if they start at the settings page and then have to go back to the integrations page...)
The video illustrates these workflows -- in the first part of the video my surface didn't connect because the module was missing, so I add the module. In the last part the module was disabled, which is instantly apparent. The middle part of the video shows how the layout changes with screen-width changes (and appearance of the "open settings" button).
Commit notes:
The PR is split into several commits because some of the changes here can be applied more generally. The commits are:
Comment: All settings pages would probably benefit from rewriting as Containers with rows & columns instead of tables...
Summary by CodeRabbit
New Features
UI/UX Improvements
✏️ Tip: You can customize this high-level summary in your review settings.