Conversation
|
🚨 End to end tests failed. Please check the failed workflow run. |
2c22e51 to
5a509d5
Compare
pinkisemils
left a comment
There was a problem hiding this comment.
On first run, the packet tunnel failed to start.
@pinkisemils made 1 comment.
Reviewable status: 0 of 64 files reviewed, all discussions resolved.
pinkisemils
left a comment
There was a problem hiding this comment.
And sometimes the tunnel monitor gets stuck in a state - I wonder if this is due to it not properly stopping to listen for the old tunnel profile or seeing updates from the old packet tunnel after the new one had already started. Could we detect that better? Probably best resolved in a new PR.
@pinkisemils made 1 comment.
Reviewable status: 0 of 64 files reviewed, all discussions resolved.
5a509d5 to
8e5e27c
Compare
rablador
left a comment
There was a problem hiding this comment.
I re-ran a fresh install, but no failing packet tunnel so far. Can't see how this would be related to the current PR though, unless the new .noReconnect strategy for some reason is invoked when it shouldn't.
@rablador made 1 comment.
Reviewable status: 0 of 64 files reviewed, all discussions resolved.
mojganii
left a comment
There was a problem hiding this comment.
Info button at Local network sharing doesn't work
@mojganii made 1 comment.
Reviewable status: 0 of 64 files reviewed, all discussions resolved.
mojganii
left a comment
There was a problem hiding this comment.
I wonder if we still keep the switches disabled when it's already enabled, shouldn't the checkmark be marked by default when it's enabled?
@mojganii made 1 comment.
Reviewable status: 0 of 64 files reviewed, all discussions resolved.
mojganii
left a comment
There was a problem hiding this comment.
@mojganii made 1 comment.
Reviewable status: 0 of 64 files reviewed, 1 unresolved discussion (waiting on @rablador).
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsViewModel.swift line 239 at r2 (raw file):
completion: @escaping () -> Void ) -> MullvadAlert { let message = [
I get the alert even when the VPN is not connected.
mojganii
left a comment
There was a problem hiding this comment.
@mojganii made 1 comment.
Reviewable status: 0 of 64 files reviewed, 2 unresolved discussions (waiting on @rablador).
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsViewModel.swift line 145 at r2 (raw file):
MullvadAlert.Action( type: .default, title: "Got it!",
is it part of the spect to allow users to enable the IAN while user doesn't enable the notification?
8e5e27c to
0947997
Compare
rablador
left a comment
There was a problem hiding this comment.
The info button was only clickable when the feature is enabled. Fixed now.
Regarding the states, good question. My thought was that consent is the overarching failsafe, so if consent is turned off the feature is turned off too. But perhaps we should automatically set switches to off then too? @pinkisemils @waahlnaden ?
@rablador made 3 comments.
Reviewable status: 0 of 66 files reviewed, 2 unresolved discussions (waiting on @mojganii).
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsViewModel.swift line 145 at r2 (raw file):
Previously, mojganii wrote…
is it part of the spect to allow users to enable the IAN while user doesn't enable the notification?
Yes, they can decline the notification and still enable IAN.
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsViewModel.swift line 239 at r2 (raw file):
Previously, mojganii wrote…
I get the alert even when the VPN is not connected.
Ah, forgot to add the conditon. Fixed now. I the for tunne state being "secured", just like in the language alert PR. If we decided to change the condition for showing the alert there, I'll update the condition likewise here too.
rablador
left a comment
There was a problem hiding this comment.
@rablador made 1 comment.
Reviewable status: 0 of 66 files reviewed, 3 unresolved discussions (waiting on @mojganii).
ios/MullvadVPN/Coordinators/Settings/Views/SettingsInfoView.swift line 1 at r3 (raw file):
//
The changes in this file relate to https://linear.app/mullvad/issue/IOS-1139/fix-the-gap-between-images-in-daita-settings.
d7fb79d to
b3f0ba5
Compare
mojganii
left a comment
There was a problem hiding this comment.
I’m wondering: if the user taps the consent checkbox again, should it become unchecked? If so, do we then disable the features that were enabled? If not, then tapping it again shouldn’t change the enabling state of the toggles.
ScreenRecording_02-06-2026 17-24-25_1.MP4
@mojganii made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 72 files reviewed, 2 unresolved discussions.
rablador
left a comment
There was a problem hiding this comment.
We decided today that the checkbox is a one-time only interaction. It will not be tappable again.
@rablador made 1 comment.
Reviewable status: 0 of 72 files reviewed, 2 unresolved discussions (waiting on @mojganii).
mojganii
left a comment
There was a problem hiding this comment.
@mojganii made 5 comments.
Reviewable status: 0 of 72 files reviewed, 7 unresolved discussions (waiting on @rablador).
ios/MullvadSettings/InclueAllNetworksSettings.swift line 33 at r6 (raw file):
} public struct InclueAllNetworksSettings: Codable, Equatable, Sendable {
typo: Inclue
ios/MullvadSettings/InclueAllNetworksSettings.swift line 36 at r6 (raw file):
public var includeAllNetworksState: InclueAllNetworksState public var localNetworkSharingState: LocalNetworkSharingState public var consent: Bool
do we really need to save is as part of TunnelSettings? I think this flag can reside in AppPreference
ios/MullvadSettings/SettingsManager.swift line 154 at r6 (raw file):
.ipOverrides, .customRelayLists, .recentConnections,
as I remember, we were not supposed to remove recents with logging out. is there any particular reason hrere?
ios/MullvadSettings/TunnelSettingsStrategy.swift line 38 at r6 (raw file):
) -> TunnelSettingsReconnectionStrategy { // Don't reconnect the tunnel If IAN consent was the setting that triggered the settings update. if oldSettings.includeAllNetworks.consent != newSettings.includeAllNetworks.consent {
I think the consent and the logic around it should live on the UI side. We only need to care about the values of the flags, and ensure users couldn't enable them without giving consent in advance.
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsView.swift line 160 at r6 (raw file):
ExternalLinkView( url: blogUrl, label: "For details, please see our blog post",
I see it as the footer on the design. which one is the reference?
b3f0ba5 to
b19eaa5
Compare
b19eaa5 to
cdf0267
Compare
rablador
left a comment
There was a problem hiding this comment.
@rablador made 5 comments.
Reviewable status: 0 of 73 files reviewed, 7 unresolved discussions (waiting on @mojganii).
ios/MullvadSettings/TunnelSettingsStrategy.swift line 38 at r6 (raw file):
Previously, mojganii wrote…
I think the consent and the logic around it should live on the UI side. We only need to care about the values of the flags, and ensure users couldn't enable them without giving consent in advance.
Consent is now part of app preferences instead, so this check has been removed.
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsView.swift line 160 at r6 (raw file):
Previously, mojganii wrote…
I see it as the footer on the design. which one is the reference?
This is a link on page 2 in the IAN view.
ios/MullvadSettings/SettingsManager.swift line 154 at r6 (raw file):
Previously, mojganii wrote…
as I remember, we were not supposed to remove recents with logging out. is there any particular reason hrere?
No, my bad. Removed.
ios/MullvadSettings/InclueAllNetworksSettings.swift line 33 at r6 (raw file):
Previously, mojganii wrote…
typo: Inclue
Oof, a bad one at that! Fixed.
ios/MullvadSettings/InclueAllNetworksSettings.swift line 36 at r6 (raw file):
Previously, mojganii wrote…
do we really need to save is as part of
TunnelSettings? I think this flag can reside inAppPreference
I agree. It was bound to the other settings before, but now we only check for it once in the IAN view.
mojganii
left a comment
There was a problem hiding this comment.
this issue might be irrelevant to this feature but after pressing Done button the logger says ' Dismissing .daita.'
ScreenRecording_02-09-2026 16-06-14_1.MP4
@mojganii reviewed 3 files, made 9 comments, and resolved 5 discussions.
Reviewable status: 3 of 73 files reviewed, 10 unresolved discussions (waiting on @rablador).
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsView.swift line 100 at r7 (raw file):
} func showEnableNofiticationsAlert() {
typo: Notifications
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsView.swift line 104 at r7 (raw file):
// two alerts with no delay between them, add a short delay // here. DispatchQueue.main.asyncAfter(deadline: .now() + 0.2) {
what is the case might end up to showing two alerts?
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsView.swift line 111 at r7 (raw file):
} func showReconsiderNofiticationsAlert() {
typo: Notifications
ios/MullvadSettings/TunnelSettingsStrategy.swift line 37 at r7 (raw file):
newSettings: LatestTunnelSettings ) -> TunnelSettingsReconnectionStrategy { if oldSettings.includeAllNetworks.localNetworkSharingState
can't we simply it here?
Code snippet:
switch (oldSettings, newSettings) {
case let (old, new) where old.includeAllNetworks != new.includeAllNetworks:
return .hardReconnect
case let (old, new) where old != new:
return .newRelayReconnect
default:
return .currentRelayReconnect
}ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsViewModel.swift line 21 at r7 (raw file):
var consent: Bool { get set } var showEnableNotificationsAlert: Bool { get set }
the doesn't imply it's a bool value
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsViewModel.swift line 22 at r7 (raw file):
var showEnableNotificationsAlert: Bool { get set } var showReconsiderNotificationsAlert: Bool { get set }
the doesn't imply it's a bool value
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsViewModel.swift line 23 at r7 (raw file):
var showEnableNotificationsAlert: Bool { get set } var showReconsiderNotificationsAlert: Bool { get set } var tunnelIsSecured: Bool { get }
isTunnelSecured?
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsViewModel.swift line 209 at r7 (raw file):
let ipList = [ " ∙ 10.0.0.0/8",
nit:we could keep private ips with the built in types out side the object.
5413228 to
eac976f
Compare
rablador
left a comment
There was a problem hiding this comment.
Good catch, fixed!
@rablador made 9 comments and resolved 2 discussions.
Reviewable status: 1 of 73 files reviewed, 8 unresolved discussions (waiting on @mojganii).
ios/MullvadSettings/TunnelSettingsStrategy.swift line 37 at r7 (raw file):
Previously, mojganii wrote…
can't we simply it here?
You're right. Now that we use a shared container object for them.
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsView.swift line 100 at r7 (raw file):
Previously, mojganii wrote…
typo: Notifications
Done.
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsView.swift line 104 at r7 (raw file):
Previously, mojganii wrote…
what is the case might end up to showing two alerts?
The leak warning and if notifications are disabled.
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsView.swift line 111 at r7 (raw file):
Previously, mojganii wrote…
typo: Notifications
Done.
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsViewModel.swift line 21 at r7 (raw file):
Previously, mojganii wrote…
the doesn't imply it's a bool value
Done.
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsViewModel.swift line 22 at r7 (raw file):
Previously, mojganii wrote…
the doesn't imply it's a bool value
Done.
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsViewModel.swift line 23 at r7 (raw file):
Previously, mojganii wrote…
isTunnelSecured?
I much prefer keeping readability, ie "if tunnelIsSecured" feels more readable than "if isTunnelSecured" to me.
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsViewModel.swift line 209 at r7 (raw file):
Previously, mojganii wrote…
nit:we could keep private ips with the built in types out side the object.
Hmm, I see your point, but doing so would need me to first create address objects from strings, then convert the address data back to strings again. It's not worth it, is it?
mojganii
left a comment
There was a problem hiding this comment.
PR looks good! It just needs to be rebased and updated to use UNUserNotificationCenter.isAllowed and UNUserNotificationCenter.isDisabled to be in sync with main.
@mojganii partially reviewed 10 files, made 2 comments, and resolved 6 discussions.
Reviewable status: 11 of 73 files reviewed, 2 unresolved discussions (waiting on @rablador).
ios/MullvadVPN/Coordinators/Settings/IncludeAllNetworks/IncludeAllNetworksSettingsViewModel.swift line 209 at r7 (raw file):
Previously, rablador (Jon Petersson) wrote…
Hmm, I see your point, but doing so would need me to first create address objects from strings, then convert the address data back to strings again. It's not worth it, is it?
the whole point is keeping all is one place if we need to have or tweak them later . but it's not critical to make this change. they are private ips and
rablador
left a comment
There was a problem hiding this comment.
@rablador resolved 2 discussions.
Reviewable status: 11 of 73 files reviewed, all discussions resolved (waiting on @mojganii).
eac976f to
ad855a9
Compare
pinkisemils
left a comment
There was a problem hiding this comment.
@pinkisemils reviewed 2 files and made 1 comment.
Reviewable status: 9 of 73 files reviewed, all discussions resolved (waiting on @mojganii).
ios/MullvadSettings/AppPreferences.swift line 63 at r9 (raw file):
@AppStorage(key: AppStorageKey.includeAllNetworksConsent.rawValue, container: .standard) public var includeAllNetworksConsent = false
This will clash with Mojgan's changes.
pinkisemils
left a comment
There was a problem hiding this comment.
@pinkisemils partially reviewed 14 files and all commit messages.
Reviewable status: 16 of 73 files reviewed, all discussions resolved (waiting on @mojganii).
ad855a9 to
6e00db1
Compare
rablador
left a comment
There was a problem hiding this comment.
👍
@rablador made 1 comment.
Reviewable status: 16 of 73 files reviewed, all discussions resolved (waiting on @mojganii).
pinkisemils
left a comment
There was a problem hiding this comment.
@pinkisemils reviewed 57 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved.
57e93ff to
58144f5
Compare
58144f5 to
e9b8154
Compare
mojganii
left a comment
There was a problem hiding this comment.
@mojganii reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: 68 of 73 files reviewed, 1 unresolved discussion (waiting on pinkisemils and rablador).
ios/MullvadVPN/Coordinators/Settings/SettingsViewControllerFactory.swift line 164 at r11 (raw file):
let viewModel = IncludeAllNetworksSettingsViewModelImpl( tunnelManager: interactorFactory.tunnelManager, appPreferences: appPreferences
nit: we could get notified the consent value changes like what we are doing in makeNotificationSettingsCoordinator to aggregate all storing in Application coordinator
rablador
left a comment
There was a problem hiding this comment.
@rablador made 1 comment.
Reviewable status: 68 of 73 files reviewed, 1 unresolved discussion (waiting on mojganii and pinkisemils).
ios/MullvadVPN/Coordinators/Settings/SettingsViewControllerFactory.swift line 164 at r11 (raw file):
Previously, mojganii wrote…
nit: we could get notified the consent value changes like what we are doing in
makeNotificationSettingsCoordinatorto aggregate all storing inApplication coordinator
Sunce the keychain changes are made in the view model as well, I'd rather keep all settings manipulation in one place if possible. Also, there's nothing to notify by enabling consent, so it would mean creating a whole callback chain for just storing one value.



Acceptance criteria
How should this be tested?
This change is