Skip to content

Commit 10735bb

Browse files
committed
feat: batch B — data-loss guards, query timeout, banner mutex
Confirmed-broken behaviors from the deep audit: - **F5 mid-edit data loss**: refresh on a dirty tab silently dropped every pending edit. on_refresh_active_tab now opens a destructive AlertDialog (Cancel | Discard and refresh) when the tab tracker has pending changes, mirroring the close-with-pending pattern. - **Disconnect with pending no warn**: clicking Disconnect with any tracker dirty silently destroyed the work. on_disconnect now prompts; "Discard and disconnect" routes through a new AppMsg::ForceDisconnect that calls do_disconnect (the previous body, factored out). - **Query timeout**: editor's Run had no wall-clock cap; runaway queries pinned the UI. New `query_timeout_secs` preference (default 60s, 0 disables); editor's tokio::select! arm races against the timeout and emits ShowTimedOut on expiry. Surfaced in Preferences → Editor as a SpinRow with subtitle. - **Banner stacking**: read-only / no-PK / pending-changes banners could all reveal at once, wasting vertical space and dulling user attention. New `refresh_banner_visibility` enforces single-banner mutual exclusion in priority order (read-only > no-PK > pending); every toggle site routes through it. Triaged as already-fixed and skipped: MySQL tinyint(1) bool detection (grid.rs:596), Postgres multi-attribute alter (structure_tracker materialize collapses), hourly history-prune timer (correct GTK pattern, no leak). ComboBoxText kept intentionally for free-text type entry.
1 parent e8f7097 commit 10735bb

7 files changed

Lines changed: 188 additions & 6 deletions

File tree

linux/crates/app/src/services/preferences.rs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,19 +9,32 @@ pub struct Preferences {
99
pub editor_font_size: u32,
1010
#[serde(default = "default_history_retention_days")]
1111
pub history_retention_days: u32,
12+
/// Wall-clock seconds before the editor's Run cancels a query
13+
/// the driver hasn't returned from. `0` disables the timeout.
14+
/// Defaults to 60s — long enough for typical OLTP work and
15+
/// catalog browsing, short enough that a runaway DDL or
16+
/// cross-join doesn't pin the GTK main thread waiting on
17+
/// shutdown.
18+
#[serde(default = "default_query_timeout_secs")]
19+
pub query_timeout_secs: u32,
1220
}
1321

1422
fn default_history_retention_days() -> u32 {
1523
30
1624
}
1725

26+
fn default_query_timeout_secs() -> u32 {
27+
60
28+
}
29+
1830
impl Default for Preferences {
1931
fn default() -> Self {
2032
Self {
2133
default_page_size: 1_000,
2234
confirm_destructive: true,
2335
editor_font_size: 12,
2436
history_retention_days: default_history_retention_days(),
37+
query_timeout_secs: default_query_timeout_secs(),
2538
}
2639
}
2740
}

linux/crates/app/src/ui/app/browse.rs

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use relm4::adw::prelude::*;
22
use relm4::gtk::gio;
3-
use relm4::{ComponentController, ComponentSender, gtk};
3+
use relm4::{ComponentController, ComponentSender, adw, gtk};
44

55
use tablepro_core::{ColumnInfo, QueryResult};
66
use uuid::Uuid;
@@ -268,8 +268,39 @@ impl App {
268268
}
269269

270270
pub(super) fn on_refresh_active_tab(&self) {
271-
if let Some(id) = self.selected_browse_tab_id() {
271+
let Some(id) = self.selected_browse_tab_id() else {
272+
return;
273+
};
274+
let dirty = crate::services::change_tracker::with_tab_ref(id, |tr| tr.has_pending()).unwrap_or(false);
275+
if !dirty {
272276
self.dispatch_to_tab(id, BrowseTabInput::Refresh);
277+
return;
273278
}
279+
// F5 mid-edit: a refetch overwrites the model and silently
280+
// drops every pending row edit / insert / delete. Confirm
281+
// with a destructive AlertDialog mirroring the close-with-
282+
// pending path so the user has to opt in to the data loss.
283+
let dialog = adw::AlertDialog::new(
284+
Some(&crate::tr!("Discard pending changes?")),
285+
Some(&crate::tr!(
286+
"Refreshing reloads the table from the database and drops every unsaved edit on this tab."
287+
)),
288+
);
289+
dialog.add_response("cancel", &crate::tr!("Cancel"));
290+
dialog.add_response("discard", &crate::tr!("Discard and refresh"));
291+
dialog.set_response_appearance("discard", adw::ResponseAppearance::Destructive);
292+
dialog.set_default_response(Some("cancel"));
293+
dialog.set_close_response("cancel");
294+
let workspace_tabs = self.workspace_tabs.clone();
295+
dialog.connect_response(None, move |dlg: &adw::AlertDialog, response: &str| {
296+
dlg.close();
297+
if response == "discard" {
298+
crate::services::change_tracker::with_tab(id, |t| t.clear());
299+
if let Some(controller) = workspace_tabs.borrow().get(&id).and_then(|t| t.browse_controller()) {
300+
let _ = controller.sender().send(BrowseTabInput::Refresh);
301+
}
302+
}
303+
});
304+
dialog.present(Some(&self.window));
274305
}
275306
}

linux/crates/app/src/ui/app/connection.rs

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,43 @@ impl App {
5454
}
5555

5656
pub(super) fn on_disconnect(&mut self, sender: ComponentSender<Self>) {
57+
// Block disconnect when any tab has pending changes. The
58+
// teardown below clears all tracker registries, so dropping
59+
// the connection mid-edit silently destroys the user's work.
60+
// Confirm via an AlertDialog mirroring the window-close-with-
61+
// pending and F5-with-pending paths.
62+
let has_pending = crate::services::change_tracker::any_pending_globally()
63+
|| crate::services::structure_tracker::any_pending_globally();
64+
if has_pending {
65+
let dialog = adw::AlertDialog::new(
66+
Some(&crate::tr!("Discard pending changes?")),
67+
Some(&crate::tr!(
68+
"Disconnecting will close every tab and drop every unsaved row edit and DDL change."
69+
)),
70+
);
71+
dialog.add_response("cancel", &crate::tr!("Cancel"));
72+
dialog.add_response("discard", &crate::tr!("Discard and disconnect"));
73+
dialog.set_response_appearance("discard", adw::ResponseAppearance::Destructive);
74+
dialog.set_default_response(Some("cancel"));
75+
dialog.set_close_response("cancel");
76+
let sender_for_resp = sender.clone();
77+
dialog.connect_response(None, move |dlg, response| {
78+
dlg.close();
79+
if response == "discard" {
80+
sender_for_resp.input(AppMsg::ForceDisconnect);
81+
}
82+
});
83+
dialog.present(Some(&self.window));
84+
return;
85+
}
86+
self.do_disconnect(sender);
87+
}
88+
89+
/// Skip the dirty check and tear the connection down. Reachable
90+
/// either from the AlertDialog "Discard and disconnect" branch
91+
/// or from a clean `Disconnect` when no tracker has pending
92+
/// changes.
93+
pub(super) fn do_disconnect(&mut self, sender: ComponentSender<Self>) {
5794
// Persist + tear down workspace tabs before dropping the
5895
// connection (persist needs the active connection_id).
5996
self.teardown_workspace_tabs();

linux/crates/app/src/ui/app/mod.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,10 @@ pub enum AppMsg {
289289
OpenHistoryQuery(String),
290290
ReplaceActiveTabQuery(String),
291291
Disconnect,
292+
/// Skip the dirty-state confirmation and tear the connection
293+
/// down immediately. Fired from the disconnect-with-pending
294+
/// AlertDialog's "Discard and disconnect" response.
295+
ForceDisconnect,
292296
PollHealth,
293297
RefreshPage,
294298
ShowShortcuts,
@@ -1201,6 +1205,7 @@ impl SimpleComponent for App {
12011205
AppMsg::OpenConnect => self.on_open_connect(sender),
12021206
AppMsg::Connected { tables, driver_id } => self.on_connected(tables, driver_id, sender),
12031207
AppMsg::Disconnect => self.on_disconnect(sender),
1208+
AppMsg::ForceDisconnect => self.do_disconnect(sender),
12041209
AppMsg::DialogClosed => self.dialog = None,
12051210
AppMsg::SelectTable {
12061211
schema,

linux/crates/app/src/ui/browse_tab.rs

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -472,7 +472,31 @@ impl BrowseTab {
472472
};
473473
self.pending_banner.set_title(&banner_title);
474474
}
475-
self.pending_banner.set_revealed(visible);
475+
self.refresh_banner_visibility(visible);
476+
}
477+
478+
/// Reveal at most ONE banner at a time. Stacking three banners
479+
/// (read-only + no-PK + pending) wastes vertical space and
480+
/// creates a wall-of-warnings aesthetic that desensitises the
481+
/// user to actual problems. Priority order, most-blocking first:
482+
///
483+
/// 1. Read-only — every other state is moot if edits are off.
484+
/// 2. No primary key — edits are blocked at row-identity level.
485+
/// 3. Pending unsaved changes — informational, only meaningful
486+
/// when the previous two are clear.
487+
///
488+
/// `pending_visible` is the caller's intended pending-banner
489+
/// state (it carries the per-tab pending-count so this helper
490+
/// doesn't need to recompute it).
491+
fn refresh_banner_visibility(&self, pending_visible: bool) {
492+
let read_only = self.read_only;
493+
let no_pk = self.current_columns.iter().any(|c| !c.primary_key)
494+
&& !self.current_columns.is_empty()
495+
&& !self.current_columns.iter().any(|c| c.primary_key);
496+
self.read_only_banner.set_revealed(read_only);
497+
self.no_pk_banner.set_revealed(!read_only && no_pk);
498+
self.pending_banner
499+
.set_revealed(!read_only && !no_pk && pending_visible);
476500
}
477501

478502
/// Walk the selection → FilterListModel → ListStore chain to
@@ -931,7 +955,15 @@ impl BrowseTab {
931955
self.delete_button
932956
.set_tooltip_text(Some(&crate::tr!("Delete selected row")));
933957
}
934-
self.no_pk_banner.set_revealed(has_columns && !has_pk);
958+
// Banner mutual exclusion lives in `refresh_banner_visibility`;
959+
// forward the current pending-banner intent so the no-PK
960+
// toggle doesn't override it. The pending banner reveals
961+
// when there's an actual count > 0.
962+
let pending_visible =
963+
crate::services::change_tracker::with_tab_ref(self.tab_id, |tr| tr.pending_count() > 0).unwrap_or(false);
964+
self.refresh_banner_visibility(pending_visible);
965+
let _ = has_columns;
966+
let _ = has_pk;
935967
}
936968

937969
fn update_paginator_label(&self) {
@@ -1479,7 +1511,10 @@ impl SimpleComponent for BrowseTab {
14791511
}
14801512
BrowseTabInput::SetReadOnly(read_only) => {
14811513
self.read_only = read_only;
1482-
self.read_only_banner.set_revealed(read_only);
1514+
let pending_visible =
1515+
crate::services::change_tracker::with_tab_ref(self.tab_id, |tr| tr.pending_count() > 0)
1516+
.unwrap_or(false);
1517+
self.refresh_banner_visibility(pending_visible);
14831518
self.refresh_crud_buttons();
14841519
// Force a full grid rebuild so editable labels turn into
14851520
// read-only labels (or vice versa) without waiting for the

linux/crates/app/src/ui/editor.rs

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,10 @@ pub enum SqlEditorInput {
6767
/// vs. sub-tabs) based on Vec length.
6868
ShowOutcomes(Vec<StatementOutcome>),
6969
ShowCancelled,
70+
/// Query exceeded the configured wall-clock timeout. Treated
71+
/// like a manual cancel from the user's perspective but with
72+
/// a different status / history-record reason.
73+
ShowTimedOut(u32),
7074
ReplaceQuery(String),
7175
}
7276

@@ -311,14 +315,28 @@ impl SimpleComponent for SqlEditor {
311315
self.executing_metadata = database_service::instance().active_metadata();
312316
self.executing_started_at = Some(SystemTime::now());
313317

318+
let timeout_secs = crate::services::preferences::load().query_timeout_secs;
314319
let sender_clone = sender.clone();
315320
sender.command(move |_, shutdown| {
316321
shutdown
317322
.register(async move {
318323
let statements = split_sql_statements(&trimmed);
324+
// A `query_timeout_secs == 0` user opt-out
325+
// turns the timeout branch off by holding a
326+
// future that never resolves. Otherwise the
327+
// tokio sleep races against `cancelled()`
328+
// and `run_statements()`; first to finish
329+
// wins.
330+
let timeout: std::pin::Pin<Box<dyn std::future::Future<Output = ()> + Send>> =
331+
if timeout_secs > 0 {
332+
Box::pin(tokio::time::sleep(std::time::Duration::from_secs(timeout_secs as u64)))
333+
} else {
334+
Box::pin(std::future::pending::<()>())
335+
};
319336
let msg = tokio::select! {
320337
biased;
321338
_ = token.cancelled() => SqlEditorInput::ShowCancelled,
339+
_ = timeout => SqlEditorInput::ShowTimedOut(timeout_secs),
322340
outcomes = run_statements(conn, statements) => {
323341
let total_ms: u128 = outcomes.iter().map(|o| o.elapsed_ms).sum();
324342
let n_ok = outcomes
@@ -409,6 +427,32 @@ impl SimpleComponent for SqlEditor {
409427
self.results_holder.append(&cancelled_page);
410428
}
411429

430+
SqlEditorInput::ShowTimedOut(secs) => {
431+
self.cancel_token = None;
432+
self.run_button.set_sensitive(true);
433+
self.cancel_button.set_visible(false);
434+
self.running_spinner.set_visible(false);
435+
let _ = sender.output(SqlEditorOutput::RunStateChanged(false));
436+
let elapsed = self
437+
.executing_started_at
438+
.and_then(|t| SystemTime::now().duration_since(t).ok())
439+
.map(|d| d.as_millis() as i64)
440+
.unwrap_or(0);
441+
let secs_str = secs.to_string();
442+
let reason =
443+
crate::tr!("Query exceeded the {n}s timeout configured in Preferences.").replace("{n}", &secs_str);
444+
self.record_history(elapsed, None, Outcome::Error(reason.clone()));
445+
self.status.set_label(&crate::tr!("timed out"));
446+
clear_box(&self.results_holder);
447+
let page = adw::StatusPage::builder()
448+
.title(crate::tr!("Query timed out"))
449+
.description(&reason)
450+
.icon_name("appointment-soon-symbolic")
451+
.vexpand(true)
452+
.build();
453+
self.results_holder.append(&page);
454+
}
455+
412456
SqlEditorInput::ReplaceQuery(text) => {
413457
self.source_view.buffer().set_text(&text);
414458
}

linux/crates/app/src/ui/preferences.rs

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,8 +126,19 @@ pub fn present(parent: &impl IsA<gtk::Widget>) {
126126
let font_size_row = adw::SpinRow::with_range(8.0, 32.0, 1.0);
127127
font_size_row.set_title(&crate::tr!("Editor font size"));
128128
font_size_row.set_value(current.editor_font_size as f64);
129-
130129
editor_group.add(&font_size_row);
130+
131+
// 0 disables, 1..=3600s allowed range. Subtitle exposes the
132+
// disable-via-zero contract so power users editing long-running
133+
// analytical queries can opt out without spelunking the JSON.
134+
let timeout_row = adw::SpinRow::with_range(0.0, 3600.0, 5.0);
135+
timeout_row.set_title(&crate::tr!("Query timeout (seconds)"));
136+
timeout_row.set_subtitle(&crate::tr!(
137+
"Cancel long-running queries automatically. Set to 0 to disable."
138+
));
139+
timeout_row.set_value(current.query_timeout_secs as f64);
140+
editor_group.add(&timeout_row);
141+
131142
editor.add(&editor_group);
132143

133144
window.add(&general);
@@ -142,12 +153,14 @@ pub fn present(parent: &impl IsA<gtk::Widget>) {
142153
let confirm = confirm_row.clone();
143154
let font = font_size_row.clone();
144155
let retention = retention_row.clone();
156+
let timeout = timeout_row.clone();
145157
std::rc::Rc::new(move || {
146158
preferences::save(&Preferences {
147159
default_page_size: page_size.value() as u64,
148160
confirm_destructive: confirm.is_active(),
149161
editor_font_size: font.value() as u32,
150162
history_retention_days: retention.value() as u32,
163+
query_timeout_secs: timeout.value() as u32,
151164
});
152165
})
153166
};
@@ -163,6 +176,10 @@ pub fn present(parent: &impl IsA<gtk::Widget>) {
163176
let s = save_all.clone();
164177
move |_| s()
165178
});
179+
timeout_row.connect_value_notify({
180+
let s = save_all.clone();
181+
move |_| s()
182+
});
166183
confirm_row.connect_active_notify({
167184
let s = save_all.clone();
168185
move |_| s()

0 commit comments

Comments
 (0)