Type: feature enhancement Reported by: AJ (field use)
Problem
Closing BCC with unsaved edits raises the discard prompt, but it doesn't say what is unsaved:
# bcc.py — MainWindow._confirm_discardQMessageBox.question(self,"Discard changes?","You have unsaved changes. Discard them?")
The status bar likewise only appends the bare string " · unsaved changes" (_update_status). So at the exact moment the decision matters — keep or throw away — there is no way to see which servers changed, or whether the pending edit was the deliberate one or an accidental cell edit. And today Discard is the only way out of the prompt: the choices are lose the work or cancel and go hunting for the Save button.
Same gap on the other paths that call _confirm_discard(): profile switch (load_profile, ~L2531), reload (~L3196), restore-from-backup (~L3196).
Requested behavior
Pending-changes list in the close/discard prompt: a plain-English summary of the diff between the in-memory server list and the baseline loaded from disk, e.g.
Expandable detail — a "Show details" disclosure revealing the masked unified JSON diff, so the exact edit is inspectable without leaving the dialog.
Save & close — prompt buttons become Save & close / Discard & close / Cancel. This is the part that bites most; Yes/No forces a discard-or-abort choice.
Implementation notes
Most of the machinery already exists — this is mostly wiring, not new invention.
Baseline.MainWindow.full_config holds the config as loaded (refreshed after every write, _loaded_stat alongside it). Diff the current self.servers against core.extract_servers(self.full_config). Careful: full_config is also the base for the stale-file merge path (~L3098-3149) — the pending-changes baseline should be the loaded server snapshot, so consider capturing an explicit self._baseline_servers at load/save time rather than deriving it, to avoid the two concerns drifting apart.
Diff rendering. Reuse bcc_core._server_sections() + _redact_servers_block(), exactly as backup_diff() does. A pending_changes_diff(baseline_cfg, current_cfg) alongside backup_diff is the natural home.
🔒 Secret masking is mandatory on this new surface. This is the fourth diff surface in the codebase, and masking has drifted on new surfaces before (see PR #14 — backup_diff rendered raw env values until it was caught in review; PR #15's stale-diff got it right by carrying the fix forward). Both the summary line and the expanded diff must go through _redact_servers_block: name the changed env keys, never the values. Ship a test asserting a known secret value is absent from the output and MASK is present.
Pure core function. Put the change computation in bcc_core as a pure function returning structured records (e.g. ChangeRecord(kind="added"|"removed"|"modified"|"enabled"|"disabled"|"renamed", name, fields)), so it is unit-testable without Qt and reusable by the GUI, by diagnostics_text, and by the status bar.
Also worth doing (same feature, cheap once the above lands)
Label the undo stack._push_undo() (~L2432) snapshots self.servers with no description, and _undo() just reports "Undone.". Threading an action label through _push_undo("Pasted 3 servers") makes the existing Undo button self-documenting and gives a chronological action log to sit next to the net-state summary. Low cost, and it answers the "history, or an undo if you will" half of the request directly.
Status bar counter._update_status could say · 3 unsaved changes instead of · unsaved changes once the count is computed — the number alone is useful ambient feedback.
Acceptance criteria
Pure, Qt-free change-computation function in bcc_core with unit tests covering add / remove / modify / rename / enable-disable.
Test asserting no raw secret value appears in either the summary or the expanded diff.
Close prompt shows the summary, offers Save & close / Discard & close / Cancel; Cancel still calls e.ignore().
The same summary appears on the other _confirm_discard() callers (profile switch, reload, restore).
Undo snapshots carry a label; _undo() reports it in the status bar.
**Type:** feature enhancement
**Reported by:** AJ (field use)
## Problem
Closing BCC with unsaved edits raises the discard prompt, but it doesn't say *what* is unsaved:
```python
# bcc.py — MainWindow._confirm_discard
QMessageBox.question(self, "Discard changes?", "You have unsaved changes. Discard them?")
```
The status bar likewise only appends the bare string `" · unsaved changes"` (`_update_status`). So at the exact moment the decision matters — keep or throw away — there is no way to see which servers changed, or whether the pending edit was the deliberate one or an accidental cell edit. And today **Discard is the only way out of the prompt**: the choices are lose the work or cancel and go hunting for the Save button.
Same gap on the other paths that call `_confirm_discard()`: profile switch (`load_profile`, ~L2531), reload (~L3196), restore-from-backup (~L3196).
## Requested behavior
1. **Pending-changes list** in the close/discard prompt: a plain-English summary of the diff between the in-memory server list and the baseline loaded from disk, e.g.
```
3 unsaved changes:
+ Added server "ssh-membermatters"
~ Changed "kicad" (args, env: GITEA_TOKEN)
- Disabled "wiremcp"
```
2. **Expandable detail** — a "Show details" disclosure revealing the masked unified JSON diff, so the exact edit is inspectable without leaving the dialog.
3. **Save & close** — prompt buttons become **Save & close / Discard & close / Cancel**. This is the part that bites most; `Yes/No` forces a discard-or-abort choice.
## Implementation notes
Most of the machinery already exists — this is mostly wiring, not new invention.
- **Baseline.** `MainWindow.full_config` holds the config as loaded (refreshed after every write, `_loaded_stat` alongside it). Diff the current `self.servers` against `core.extract_servers(self.full_config)`. Careful: `full_config` is also the base for the stale-file merge path (~L3098-3149) — the pending-changes baseline should be the *loaded* server snapshot, so consider capturing an explicit `self._baseline_servers` at load/save time rather than deriving it, to avoid the two concerns drifting apart.
- **Diff rendering.** Reuse `bcc_core._server_sections()` + `_redact_servers_block()`, exactly as `backup_diff()` does. A `pending_changes_diff(baseline_cfg, current_cfg)` alongside `backup_diff` is the natural home.
- **🔒 Secret masking is mandatory on this new surface.** This is the fourth diff surface in the codebase, and masking has drifted on new surfaces before (see PR #14 — `backup_diff` rendered raw env values until it was caught in review; PR #15's stale-diff got it right by carrying the fix forward). Both the summary line and the expanded diff must go through `_redact_servers_block`: name the changed env **keys**, never the values. Ship a test asserting a known secret value is absent from the output and `MASK` is present.
- **Pure core function.** Put the change computation in `bcc_core` as a pure function returning structured records (e.g. `ChangeRecord(kind="added"|"removed"|"modified"|"enabled"|"disabled"|"renamed", name, fields)`), so it is unit-testable without Qt and reusable by the GUI, by `diagnostics_text`, and by the status bar.
### Also worth doing (same feature, cheap once the above lands)
- **Label the undo stack.** `_push_undo()` (~L2432) snapshots `self.servers` with no description, and `_undo()` just reports `"Undone."`. Threading an action label through `_push_undo("Pasted 3 servers")` makes the existing Undo button self-documenting and gives a chronological action log to sit next to the net-state summary. Low cost, and it answers the "history, or an undo if you will" half of the request directly.
- **Status bar counter.** `_update_status` could say `· 3 unsaved changes` instead of `· unsaved changes` once the count is computed — the number alone is useful ambient feedback.
## Acceptance criteria
- [ ] Pure, Qt-free change-computation function in `bcc_core` with unit tests covering add / remove / modify / rename / enable-disable.
- [ ] Test asserting no raw secret value appears in either the summary or the expanded diff.
- [ ] Close prompt shows the summary, offers Save & close / Discard & close / Cancel; Cancel still calls `e.ignore()`.
- [ ] The same summary appears on the other `_confirm_discard()` callers (profile switch, reload, restore).
- [ ] Undo snapshots carry a label; `_undo()` reports it in the status bar.
the_og
added the P1 label 2026-08-12 02:36:55 -04:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Type: feature enhancement
Reported by: AJ (field use)
Problem
Closing BCC with unsaved edits raises the discard prompt, but it doesn't say what is unsaved:
The status bar likewise only appends the bare string
" · unsaved changes"(_update_status). So at the exact moment the decision matters — keep or throw away — there is no way to see which servers changed, or whether the pending edit was the deliberate one or an accidental cell edit. And today Discard is the only way out of the prompt: the choices are lose the work or cancel and go hunting for the Save button.Same gap on the other paths that call
_confirm_discard(): profile switch (load_profile, ~L2531), reload (~L3196), restore-from-backup (~L3196).Requested behavior
Pending-changes list in the close/discard prompt: a plain-English summary of the diff between the in-memory server list and the baseline loaded from disk, e.g.
Expandable detail — a "Show details" disclosure revealing the masked unified JSON diff, so the exact edit is inspectable without leaving the dialog.
Save & close — prompt buttons become Save & close / Discard & close / Cancel. This is the part that bites most;
Yes/Noforces a discard-or-abort choice.Implementation notes
Most of the machinery already exists — this is mostly wiring, not new invention.
MainWindow.full_configholds the config as loaded (refreshed after every write,_loaded_statalongside it). Diff the currentself.serversagainstcore.extract_servers(self.full_config). Careful:full_configis also the base for the stale-file merge path (~L3098-3149) — the pending-changes baseline should be the loaded server snapshot, so consider capturing an explicitself._baseline_serversat load/save time rather than deriving it, to avoid the two concerns drifting apart.bcc_core._server_sections()+_redact_servers_block(), exactly asbackup_diff()does. Apending_changes_diff(baseline_cfg, current_cfg)alongsidebackup_diffis the natural home.backup_diffrendered raw env values until it was caught in review; PR #15's stale-diff got it right by carrying the fix forward). Both the summary line and the expanded diff must go through_redact_servers_block: name the changed env keys, never the values. Ship a test asserting a known secret value is absent from the output andMASKis present.bcc_coreas a pure function returning structured records (e.g.ChangeRecord(kind="added"|"removed"|"modified"|"enabled"|"disabled"|"renamed", name, fields)), so it is unit-testable without Qt and reusable by the GUI, bydiagnostics_text, and by the status bar.Also worth doing (same feature, cheap once the above lands)
_push_undo()(~L2432) snapshotsself.serverswith no description, and_undo()just reports"Undone.". Threading an action label through_push_undo("Pasted 3 servers")makes the existing Undo button self-documenting and gives a chronological action log to sit next to the net-state summary. Low cost, and it answers the "history, or an undo if you will" half of the request directly._update_statuscould say· 3 unsaved changesinstead of· unsaved changesonce the count is computed — the number alone is useful ambient feedback.Acceptance criteria
bcc_corewith unit tests covering add / remove / modify / rename / enable-disable.e.ignore()._confirm_discard()callers (profile switch, reload, restore)._undo()reports it in the status bar.