network: prevent closing modal when configuration is in progress#1161
network: prevent closing modal when configuration is in progress#1161rvykydal wants to merge 1 commit intorhinstaller:mainfrom
Conversation
Summary of ChangesHello @rvykydal, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the user experience and robustness of the network configuration interface by preventing the main modal from being closed while network changes are in progress. It introduces a reactive mechanism to detect ongoing operations within the embedded Cockpit network manager, ensuring that users cannot prematurely dismiss the configuration dialog and potentially leave the system in an inconsistent state. This change improves the reliability of network configuration workflows. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to prevent the network configuration modal from being closed while changes are being applied. This is achieved by tracking the state of Cockpit's network manager via sessionStorage. The implementation is solid, but I've identified a couple of areas where robustness can be improved by adding error handling for sessionStorage access, which could fail if disabled in the browser, potentially crashing the component. The test files have been updated accordingly to remove obsolete time.sleep calls, which is a great improvement.
| useEffect(() => { | ||
| // Check initial states | ||
| const checkpointState = window.sessionStorage.getItem("cockpit_has_checkpoint"); | ||
| const modalState = window.sessionStorage.getItem("cockpit_has_modal"); | ||
| setHasActiveCheckpoint(checkpointState === "true"); | ||
| setHasModal(modalState === "true"); | ||
|
|
||
| const handleStorageEvent = (event) => { | ||
| if (event.key === "cockpit_has_checkpoint") { | ||
| setHasActiveCheckpoint(event.newValue === "true"); | ||
| } else if (event.key === "cockpit_has_modal") { | ||
| setHasModal(event.newValue === "true"); | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener("storage", handleStorageEvent); | ||
|
|
||
| return () => window.removeEventListener("storage", handleStorageEvent); | ||
| }, []); |
There was a problem hiding this comment.
The useNetworkStatus hook directly accesses window.sessionStorage. If session storage is disabled by the user's browser settings (e.g., by blocking all cookies), this will throw an error and crash the component. It's good practice to wrap storage access in a try...catch block to handle such cases gracefully. This will improve the robustness of the component.
useEffect(() => {
try {
// Check initial states
const checkpointState = window.sessionStorage.getItem("cockpit_has_checkpoint");
const modalState = window.sessionStorage.getItem("cockpit_has_modal");
setHasActiveCheckpoint(checkpointState === "true");
setHasModal(modalState === "true");
} catch (e) {
console.error("Could not access sessionStorage. Network status may be incorrect.", e);
return; // Don't set up listener if storage is not available
}
const handleStorageEvent = (event) => {
if (event.key === "cockpit_has_checkpoint") {
setHasActiveCheckpoint(event.newValue === "true");
} else if (event.key === "cockpit_has_modal") {
setHasModal(event.newValue === "true");
}
};
window.addEventListener("storage", handleStorageEvent);
return () => window.removeEventListener("storage", handleStorageEvent);
}, []);| useEffect(() => { | ||
| return () => { | ||
| // Clear checkpoint status on unmount to avoid stale data | ||
| // Note: cockpit_has_modal is managed by Cockpit itself, so we don't clear it | ||
| window.sessionStorage.setItem("cockpit_has_checkpoint", "false"); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
Similar to the useNetworkStatus hook, this useEffect hook accesses window.sessionStorage without error handling. If session storage is unavailable, this will throw an unhandled exception when the component unmounts. It's important to wrap this call in a try...catch block for robustness.
useEffect(() => {
return () => {
try {
// Clear checkpoint status on unmount to avoid stale data
// Note: cockpit_has_modal is managed by Cockpit itself, so we don't clear it
window.sessionStorage.setItem("cockpit_has_checkpoint", "false");
} catch (e) {
console.error("Could not access sessionStorage to clear network checkpoint status.", e);
}
};
}, []);
|
|
||
| useEffect(() => { | ||
| // Check initial states | ||
| const checkpointState = window.sessionStorage.getItem("cockpit_has_checkpoint"); |
There was a problem hiding this comment.
How long is it expected to take for the checkpoint operation on average?
There was a problem hiding this comment.
The check is done 1s after modifying the configuration, see the comment with videos below.
|
The time intervals for check, curtain and rollback are defined here Video with succeeded connection check (see the Close button): Screencast.From.2026-02-05.12-16-42.mp4Video with failed connection check: Screencast.From.2026-02-05.12-17-50.mp4 |
rvykydal
force-pushed
the
network-checkpoint-event
branch
from
afb2164 to
0610783
Compare
Resolves: INSTALLER-4594
rvykydal
force-pushed
the
network-checkpoint-event
branch
from
0610783 to
0acf802
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
useNetworkStatus, consider guarding thestorageevent handler withif (event.storageArea !== window.sessionStorage) return;so that updates from other storage areas don’t accidentally affect the modal state. - The sessionStorage keys (
"cockpit_has_checkpoint","cockpit_has_modal") are used in multiple places; extracting them into shared constants would reduce the chance of typos and make future changes easier. - On unmount you currently force
cockpit_has_checkpointto the string "false"; if the iframe is still active or another consumer is using that key this may be surprising—consider either removing the key or coordinating with the producer to avoid overwriting a legitimate value.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `useNetworkStatus`, consider guarding the `storage` event handler with `if (event.storageArea !== window.sessionStorage) return;` so that updates from other storage areas don’t accidentally affect the modal state.
- The sessionStorage keys (`"cockpit_has_checkpoint"`, `"cockpit_has_modal"`) are used in multiple places; extracting them into shared constants would reduce the chance of typos and make future changes easier.
- On unmount you currently force `cockpit_has_checkpoint` to the string "false"; if the iframe is still active or another consumer is using that key this may be surprising—consider either removing the key or coordinating with the producer to avoid overwriting a legitimate value.
## Individual Comments
### Comment 1
<location path="test/helpers/network.py" line_range="245-254" />
<code_context>
b = self.browser
b.wait_in_text(f"dt:contains('{setting_title}') + dd", setting_value)
- def wait_for_connectivity_check(self):
- # FIXME: this should wait for the Close button becoming clickable
- # based on events from cockpit module (TBD)
- time.sleep(COCKPIT_CHECKPOINT_SETTLE_TIME+0.2)
-
def set_mtu_on_iface(self, iface, mtu):
n = self
n.enter_network()
n.select_iface(iface)
n.set_mtu(mtu)
n.wait_for_iface_setting("MTU", mtu)
- n.wait_for_connectivity_check()
n.exit_network()
</code_context>
<issue_to_address>
**suggestion (testing):** Replace the removed sleep-based helper with an explicit wait on the new UI condition.
With `wait_for_connectivity_check()` removed, these tests now exit the dialog immediately after setting MTU/DNS, which can introduce flakiness. Since the UI now disables the Close button while applying changes, please add a synchronization helper that waits for a stable state, e.g. Close button re-enabled and/or the "Applying changes..." label disappearing, and call it from `set_mtu_on_iface` and `add_dns_server_to_iface` before `exit_network()`.
Suggested implementation:
```python
b = self.browser
b.wait_in_text(f"dt:contains('{setting_title}') + dd", setting_value)
def wait_for_connectivity_check(self):
"""
Wait until the network dialog finishes applying changes.
The UI disables the Close button and shows an "Applying changes..."
indicator while changes are in progress. We wait for:
* the "Applying changes..." indicator to disappear, and
* the Close button to be re-enabled.
"""
b = self.browser
def connectivity_applied():
# Still applying if the label is visible
if b.is_present("div:contains('Applying changes')"):
return False
# Wait for an enabled Close button
return b.is_present("button:contains('Close'):not([disabled])")
# Allow enough time for NM checkpoint rollback and UI settling
wait(connectivity_applied, delay=0.2, timeout=COCKPIT_CHECKPOINT_ROLLBACK_TIME + 5)
```
```python
def set_mtu_on_iface(self, iface, mtu):
n = self
n.enter_network()
n.select_iface(iface)
n.set_mtu(mtu)
n.wait_for_iface_setting("MTU", mtu)
n.wait_for_connectivity_check()
n.exit_network()
```
1. Add a corresponding `n.wait_for_connectivity_check()` call in `add_dns_server_to_iface()` just before `n.exit_network()`, mirroring the pattern used in `set_mtu_on_iface()`. The exact `SEARCH` pattern will depend on how `add_dns_server_to_iface()` is implemented (typically after the final `n.wait_for_iface_setting("DNS", ...)` or equivalent).
2. If the test browser wrapper does not support the `:not([disabled])` CSS selector or `div:contains('Applying changes')` lookup, adjust the selectors in `connectivity_applied()` to match the existing patterns used elsewhere in this test suite (for example, using specific IDs/classes for the dialog, label, and Close button).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@KKoukiou could you review please? I am considering removing the |
KKoukiou
left a comment
KKoukiou
left a comment
There was a problem hiding this comment.
Maybe you can move the useMaybeBackdrop function in src/hooks/ and we can reuse it here. There is code duplication happening.
| const handleClose = () => { | ||
| // Prevent closing if there's an active checkpoint or modal dialog |
There was a problem hiding this comment.
This is redundant, as the button is disabled using the same condition, it can't be clicked.
| setHasActiveCheckpoint(checkpointState === "true"); | ||
| setHasModal(modalState === "true"); | ||
|
|
||
| const handleStorageEvent = (event) => { |
There was a problem hiding this comment.
handleStorageEvent is a bit misleading function name here.
| return () => window.removeEventListener("storage", handleStorageEvent); | ||
| }, []); | ||
|
|
||
| const hasActiveDialog = useMemo(() => hasActiveCheckpoint || hasModal, [hasActiveCheckpoint, hasModal]); |
There was a problem hiding this comment.
The fact that a dialog is open, should not prevent the Close button to be functional IMO. This is not the behavior we have in Storage Cockpit integration page at least.
There was a problem hiding this comment.
I think from the UX pov, Cockpit's modal should be modal - ie mandatory with UI around disabled - like the backdrop for Storage, but given I am not using the backdrop I am disabling the close button of the modal embedding the iframe. Maybe I can try to add the backdrop to the Anaconda modal instead.
2 participants
Resolves: INSTALLER-4594