Skip to content

Commit dbcdd52

Browse files
martinpittmartinpitt
committed
FIXUP: Replace useEffect() with model wait_connection()
cockpit-project#22884 (comment)
1 parent b8eaf5d commit dbcdd52

File tree

2 files changed

+151
-118
lines changed

2 files changed

+151
-118
lines changed

‎pkg/networkmanager/interfaces.js‎

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1229,6 +1229,82 @@ export function NetworkManagerModel() {
12291229
const reason = priv(this).lastFailureReason;
12301230
priv(this).lastFailureReason = undefined;
12311231
return reason;
1232+
},
1233+
1234+
// Mark that a pending connection is being cancelled by the user
1235+
cancel_pending_connection: function() {
1236+
priv(this).connectionCancelled = true;
1237+
},
1238+
1239+
// Wait for a connection to complete
1240+
// For WiFi, pass expected_ssid to verify we connected to the right network
1241+
// Returns a Promise that resolves on success or cancel, rejects with {reason} on failure
1242+
wait_connection: function(expected_ssid) {
1243+
priv(this).connectionCancelled = false;
1244+
utils.debug("wait_connection: starting, iface:", this.Interface, "expected:", expected_ssid, "initial state:", this.State);
1245+
return new Promise((resolve, reject) => {
1246+
let activationStarted = false;
1247+
1248+
const cleanup = () => self.removeEventListener("changed", check);
1249+
1250+
const check = () => {
1251+
utils.debug("wait_connection check: state:", this.State, "ssid:", this.ActiveAccessPoint?.Ssid,
1252+
"activeConn:", !!this.ActiveConnection, "activationStarted:", activationStarted,
1253+
"lastFailureReason:", priv(this).lastFailureReason,
1254+
"connectionCancelled:", priv(this).connectionCancelled);
1255+
1256+
// captured a failure?
1257+
const reason = this.consume_failure_reason();
1258+
if (reason) {
1259+
cleanup();
1260+
console.warn("wait_connection: connection failed for", this.Interface, "reason:", reason);
1261+
const error = new Error("Connection failed");
1262+
error.reason = reason;
1263+
reject(error);
1264+
return;
1265+
}
1266+
1267+
// https://networkmanager.dev/docs/api/latest/nm-dbus-types.html#NMDeviceState
1268+
switch (this.State) {
1269+
case 100: // NM_DEVICE_STATE_ACTIVATED
1270+
if (!expected_ssid || this.ActiveAccessPoint?.Ssid === expected_ssid) {
1271+
utils.debug("wait_connection: success");
1272+
cleanup();
1273+
resolve();
1274+
}
1275+
break;
1276+
1277+
case 30: // NM_DEVICE_STATE_DISCONNECTED; initial state, so wait for activation to start
1278+
case 120: // NM_DEVICE_STATE_FAILED
1279+
// Disconnected/failed after activation started means user cancelled or failure without reason
1280+
if (activationStarted && !this.ActiveConnection) {
1281+
cleanup();
1282+
if (priv(this).connectionCancelled) {
1283+
utils.debug("wait_connection: cancelled by user");
1284+
resolve();
1285+
} else {
1286+
console.warn("wait_connection: connection failed for", this.Interface, "without captured reason");
1287+
reject(new Error("Connection failed"));
1288+
}
1289+
}
1290+
break;
1291+
1292+
case 20: // NM_DEVICE_STATE_UNAVAILABLE
1293+
case 110: // NM_DEVICE_STATE_DEACTIVATING
1294+
break;
1295+
1296+
// any other state means we're activating
1297+
default:
1298+
if (!activationStarted) {
1299+
utils.debug("wait_connection: activation started");
1300+
activationStarted = true;
1301+
}
1302+
}
1303+
};
1304+
1305+
self.addEventListener("changed", check);
1306+
check(); // Check current state immediately in case already connected
1307+
});
12321308
}
12331309
},
12341310

@@ -1241,7 +1317,7 @@ export function NetworkManagerModel() {
12411317
// We deduplicate by MAC first, preferring the one with a known connection
12421318
const apByMac = new Map();
12431319
(obj.AccessPoints || []).forEach(ap => {
1244-
utils.debug(`AP: Ssid '${ap.Ssid}' HwAddress: ${ap.HwAddress}) Strength: ${ap.Strength} hasConnection: ${!!ap.Connection}`);
1320+
utils.debug(`AP: Ssid '${ap.Ssid}' HwAddress: ${ap.HwAddress} Strength: ${ap.Strength} hasConnection: ${!!ap.Connection}`);
12451321
if (!apByMac.get(ap.HwAddress) || ap.Connection)
12461322
apByMac.set(ap.HwAddress, ap);
12471323
});

‎pkg/networkmanager/network-interface.jsx‎

Lines changed: 74 additions & 117 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,6 @@ const WiFiConnectDialog = ({ dev, model, ssid: knownSsid, ap }) => {
8686
const [passwordVisible, setPasswordVisible] = useState(false);
8787
const [dialogError, setDialogError] = useState(null);
8888
const [connecting, setConnecting] = useState(false);
89-
const [activeConnection, setActiveConnection] = useState(null);
9089
const [createdConnection, setCreatedConnection] = useState(null);
9190

9291
const isHidden = !knownSsid;
@@ -100,52 +99,16 @@ const WiFiConnectDialog = ({ dev, model, ssid: knownSsid, ap }) => {
10099
useEvent(model, "changed");
101100

102101
const onCancel = () => {
103-
if (connecting) {
102+
if (connecting && createdConnection) {
104103
utils.debug("Cancelling connection to", ssid);
105-
// Deactivate the pending connection
106-
if (activeConnection) {
107-
activeConnection.deactivate()
108-
.catch(err => console.warn("Failed to deactivate connection:", err));
109-
}
110-
// Delete the created connection
111-
if (createdConnection) {
112-
createdConnection.delete_()
113-
.catch(err => console.warn("Failed to delete connection:", err));
114-
}
104+
dev.cancel_pending_connection();
105+
createdConnection.delete_()
106+
.catch(err => console.warn("Failed to delete connection:", err));
115107
}
116108
Dialogs.close();
117109
};
118110

119-
// Monitor active connection state changes
120-
useEffect(() => {
121-
if (!activeConnection)
122-
return;
123-
124-
const acState = activeConnection.State;
125-
const currentSSID = dev.ActiveAccessPoint?.Ssid;
126-
127-
utils.debug("ActiveConnection state changed:", acState, "current SSID:", currentSSID, "target:", ssid);
128-
129-
// ActiveConnection states:
130-
// 0 = UNKNOWN, 1 = ACTIVATING, 2 = ACTIVATED, 3 = DEACTIVATING, 4 = DEACTIVATED
131-
132-
if (acState === 2 && currentSSID === ssid) {
133-
utils.debug("Connected successfully to", ssid);
134-
Dialogs.close();
135-
} else if (acState === 4) {
136-
utils.debug("Connection failed for", ssid);
137-
setConnecting(false);
138-
setDialogError(isHidden ? _("Failed to connect. Check your credentials.") : _("Failed to connect. Check your password."));
139-
if (createdConnection) {
140-
createdConnection.delete_()
141-
.catch(err => console.warn("Failed to delete connection:", err));
142-
}
143-
setActiveConnection(null);
144-
setCreatedConnection(null);
145-
}
146-
}, [activeConnection, activeConnection?.State, dev.ActiveAccessPoint?.Ssid, ssid, createdConnection, Dialogs, isHidden]);
147-
148-
const onSubmit = (ev) => {
111+
const onSubmit = async (ev) => {
149112
if (ev) {
150113
ev.preventDefault();
151114
}
@@ -177,16 +140,28 @@ const WiFiConnectDialog = ({ dev, model, ssid: knownSsid, ap }) => {
177140
};
178141
}
179142

180-
dev.activate_with_settings(settings, isHidden ? null : ap)
181-
.then(result => {
182-
utils.debug("Connection activation started");
183-
setCreatedConnection(result.connection);
184-
setActiveConnection(result.active_connection);
185-
})
186-
.catch(err => {
187-
setConnecting(false);
188-
setDialogError(typeof err === 'string' ? err : err.message);
189-
});
143+
let connection = null;
144+
try {
145+
// ap might be stale if there was a scan since opening the dialog, so pass NULL
146+
// NM will find the right AP by SSID
147+
const result = await dev.activate_with_settings(settings, null);
148+
connection = result.connection;
149+
utils.debug("Connection activation started");
150+
setCreatedConnection(connection);
151+
await dev.wait_connection(ssid);
152+
utils.debug("Connected successfully to", ssid);
153+
Dialogs.close();
154+
} catch (err) {
155+
setConnecting(false);
156+
setDialogError(err.reason === 7 // NM_DEVICE_STATE_REASON_NO_SECRETS
157+
? _("Failed to connect. Check your password.")
158+
: err.toString());
159+
160+
// just in case something survived, clean up
161+
connection?.delete_()
162+
.catch(err => utils.debug("Failed to delete failed connection:", err));
163+
setCreatedConnection(null);
164+
}
190165
};
191166

192167
return (
@@ -275,7 +250,6 @@ export const NetworkInterfacePage = ({
275250
const [isScanning, setIsScanning] = useState(false);
276251
const [prevAPCount, setPrevAPCount] = useState(0);
277252
const [networkSearch, setNetworkSearch] = useState("");
278-
const [pendingConnection, setPendingConnection] = useState(null); // ssid
279253

280254
const dev_name = iface.Name;
281255
const dev = iface.Device;
@@ -293,33 +267,6 @@ export const NetworkInterfacePage = ({
293267
}
294268
});
295269

296-
// Monitor device state to detect connection success/failure
297-
useEffect(() => {
298-
if (!dev || !pendingConnection)
299-
return;
300-
301-
// Successfully connected
302-
if (dev.ActiveAccessPoint?.Ssid === pendingConnection && dev.State === 100) { // NM_DEVICE_STATE_ACTIVATED
303-
setPendingConnection(null);
304-
return;
305-
}
306-
307-
// Check for failure - if there's a captured failure reason, the connection attempt failed
308-
if (dev.State === 30 && !dev.ActiveConnection) { // NM_DEVICE_STATE_DISCONNECTED
309-
const failureReason = dev.consume_failure_reason();
310-
if (failureReason !== undefined) {
311-
utils.debug("Connection to", pendingConnection, "failed with captured reason:", failureReason);
312-
show_error_dialog(
313-
cockpit.format(_("Failed to connect to $0"), pendingConnection),
314-
failureReason === 7 // NM_DEVICE_STATE_REASON_NO_SECRETS
315-
? _("Network password is not stored. Please forget and reconnect to this network.")
316-
: _("Connection failed. Check your credentials.")
317-
);
318-
setPendingConnection(null);
319-
}
320-
}
321-
}, [dev, dev?.State, dev?.StateReason, dev?.ActiveConnection, dev?.ActiveAccessPoint?.Ssid, pendingConnection]);
322-
323270
// WiFi scanning: re-enable button when APs change or after timeout
324271
useEffect(() => {
325272
if (isScanning) {
@@ -882,51 +829,61 @@ export const NetworkInterfacePage = ({
882829
}
883830
}
884831

885-
function connectToAP(ap) {
832+
async function connectToAP(ap) {
886833
// we don't show a Connect button for hidden networks
887834
cockpit.assert(ap.Ssid);
888835
utils.debug("Connecting to", ap.Ssid);
889836

890-
if (ap.Connection) {
891-
// Activate existing connection (which already has password if needed)
892-
utils.debug("Activating existing connection for", ap.Ssid);
893-
setPendingConnection(ap.Ssid);
894-
ap.Connection.activate(dev, ap)
895-
.then(() => utils.debug("Connection activation started for", ap.Ssid))
896-
.catch(error => {
897-
setPendingConnection(null);
898-
show_unexpected_error(error);
899-
});
900-
return;
901-
}
902-
903-
// Create new connection
904-
const isSecured = !!(ap.WpaFlags || ap.RsnFlags);
837+
try {
838+
if (ap.Connection) {
839+
// Activate existing connection (which already has password if needed)
840+
utils.debug("Activating existing connection for", ap.Ssid);
841+
await ap.Connection.activate(dev, ap);
842+
utils.debug("Connection activation started for", ap.Ssid);
843+
await dev.wait_connection(ap.Ssid);
844+
utils.debug("Connected successfully to", ap.Ssid);
845+
return;
846+
}
905847

906-
if (isSecured) {
907-
// Show password dialog for secured networks
908-
utils.debug("Showing password dialog for", ap.Ssid);
909-
Dialogs.show(<WiFiConnectDialog dev={dev} ap={ap} ssid={ap.Ssid} model={model} />);
910-
return;
911-
}
848+
// Create new connection
849+
const isSecured = !!(ap.WpaFlags || ap.RsnFlags);
912850

913-
// Create new connection for open networks
914-
utils.debug("Creating new connection for", ap.Ssid);
915-
const settings = {
916-
connection: {
917-
id: ap.Ssid,
918-
type: "802-11-wireless",
919-
autoconnect: true,
920-
},
921-
"802-11-wireless": {
922-
ssid: utils.ssid_to_nm(ap.Ssid),
923-
mode: "infrastructure",
851+
if (isSecured) {
852+
// Show password dialog for secured networks
853+
utils.debug("Showing password dialog for", ap.Ssid);
854+
Dialogs.show(<WiFiConnectDialog dev={dev} ap={ap} ssid={ap.Ssid} model={model} />);
855+
return;
924856
}
925-
};
926857

927-
dev.activate_with_settings(settings, ap)
928-
.then(result => utils.debug("Connected successfully to", ap.Ssid))
929-
.catch(show_unexpected_error);
858+
// Create new connection for open networks
859+
utils.debug("Creating new connection for", ap.Ssid);
860+
const settings = {
861+
connection: {
862+
id: ap.Ssid,
863+
type: "802-11-wireless",
864+
autoconnect: true,
865+
},
866+
"802-11-wireless": {
867+
ssid: utils.ssid_to_nm(ap.Ssid),
868+
mode: "infrastructure",
869+
}
870+
};
871+
872+
// Pass null for specific_object - NM will find the right AP by SSID
873+
await dev.activate_with_settings(settings, null);
874+
utils.debug("Connection activation started for", ap.Ssid);
875+
await dev.wait_connection(ap.Ssid);
876+
utils.debug("Connected successfully to", ap.Ssid);
877+
} catch (error) {
878+
// Provide context-appropriate error message
879+
const errorMsg = error.reason === 7 // NM_DEVICE_STATE_REASON_NO_SECRETS
880+
? _("Network password is not stored. Please forget and reconnect to this network.")
881+
: error.toString();
882+
show_error_dialog(
883+
cockpit.format(_("Failed to connect to $0"), ap.Ssid),
884+
errorMsg
885+
);
886+
}
930887
}
931888

932889
const networkSort = (rows, direction, columnIndex) => {

0 commit comments

Comments
 (0)