Feat/native proxy settings#32804
Conversation
github-actions
Bot
added
the
Z-Community-PR
Half-Shot
left a comment
Half-Shot
left a comment
There was a problem hiding this comment.
Hey, thanks for the contribution! Unfortunately you've caught us in the mid-transition phase to MVVM so the changes are going to need a small shuffle. I think basically this involves moving the component to shared-components, and building up a view model for interfacing with Settings.
The other bits of the code are looking good, thanks for a good start
| | "userDownloadCompleted" | ||
| | "userDownloadAction" | ||
| | "openDesktopCapturerSourcePicker" | ||
| | "open_proxy_settings" |
There was a problem hiding this comment.
This should probably follow the same casing
| | "open_proxy_settings" | |
| | "openProxySettings" |
| private renderProxySection(): ReactNode { | ||
| if (!window.electron) return null; | ||
|
|
||
| const config = SettingsStore.getValue("desktopProxyConfig") || { mode: "system" }; |
There was a problem hiding this comment.
Should { mode: "system" } just be a default value of desktopProxyConfig? It does support defaults
| } | ||
|
|
||
| private renderProxySection(): ReactNode { | ||
| if (!window.electron) return null; |
There was a problem hiding this comment.
You should use the Platform code for this. E.g. exposing a supportsProxyConfiguration() like https://github.com/vector-im/riot-web/blob/da5a30ba7130f763f9b324bb1e2c68377cf728bb/apps/web/src/BasePlatform.ts#L162-L169.
And then it's just a case of PlatformPeg.get().supportsProxyConfiguration() which is a bit more generic than checking if electron is exposed.
| * @param props - The component props. | ||
| * @param props.onFinished - Callback invoked when the modal is closed or settings are saved. | ||
| */ | ||
| export const NetworkProxyModal: React.FC<Props> = ({ onFinished }) => { |
There was a problem hiding this comment.
Hey! So we're moving to a MVVM based approach for all new code in Element Web and unfortunately you've made the critical mistake of reading our existing code. https://github.com/element-hq/element-web/blob/develop/docs/MVVM.md. Essentially I'd port the logic of this component into a NetworkProxyViewModel and the view rendering to a NeworkProxyView in shared-components.
The nice upshot of this is that you can then design the modal quite nicely in storybook without having to run a full desktop client.
| const update = (patch: Partial<ProxyConfig>): void => setConfig((prev) => ({ ...prev, ...patch })); | ||
|
|
||
| return ( | ||
| <FocusLock |
There was a problem hiding this comment.
So I think here you can just use BaseDialog, with the above suggestion about view models, I'd do:
export const NetworkProxyModal: React.FC<Props> = ({ onFinished }) => {
const vm = new NetworkProxyViewModel();
return <BaseDialog {...}>
<NetworkProxyModalView vm={vm} />
</BaseDialog>
}| "cameras": "Kameras", | ||
| "cancel": "Abbrechen", | ||
| "capabilities": "Funktionen", | ||
| "configuration": "Konfiguration", |
There was a problem hiding this comment.
Ah yeah, another one of our footguns. We don't accept translations directly into the codebase even if you know the translations, it has to go via Localazy unless it's en_EN.json.
| }, | ||
| "desktopProxyConfig": { | ||
| supportedLevels: [SettingLevel.PLATFORM], | ||
| default: { mode: "system" } as ProxyConfig, |
|
|
tim2zg
force-pushed
the
feat/native-proxy-settings
branch
2 times, most recently
from
e1d52f8 to
c5a662f
Compare
- Ported modal logic to NetworkProxyViewModel and view to NetworkProxyView in shared-components - Implemented real logic for supportsProxyConfiguration in ElectronPlatform - Renamed open_proxy_settings to openProxySettings for consistent casing - Removed German translations (to be handled via Localazy) - Registered desktopProxyConfig in SettingsStore - Simplified NetworkProxyModal by utilizing the new MVVM components and BaseDialog
tim2zg
force-pushed
the
feat/native-proxy-settings
branch
from
c5a662f to
ea1bb74
Compare
|
Thank you for the feedback and guidance. |
- Added Network Proxy link to AuthFooter for pre-login configuration - Ensured proxy settings survive logout - Improved proxy rule generation for HTTPS support - Cleaned up MVVM implementation and translations
- Ported all desktop-side proxy fixes to apps/desktop - Verified MVVM migration and auth footer entry - Ensured consistent IPC naming and platform abstraction - Fixed duplicate imports and globalThis usage in electron-main
Half-Shot
added
the
T-Feature
3 participants
Checklist
public/exportedsymbols have accurate TSDoc documentation.This is the UI part of this PR on desktop: element-hq/element-desktop#2827