Skip to content

Commit 9c54f38

Browse files
claudesinelaw
authored andcommitted
refactor(preview): fire after_file_open on escalation; single source of truth
Two related changes to the preview/after_file_open machinery. 1. Escalation now fires the deferred hook. Previewing a file defers `after_file_open`; previously, escalating that preview to a permanent buffer (editing it, Enter, focusing away) never fired it at all — so a file you preview-then-commit diverged from one you open directly, and plugin side effects keyed on the hook (csharp `dotnet restore`, the asm-lsp config offer, git_explorer refresh) silently never ran. The `promote_*` methods now fire the deferred hook at the moment of commitment, so the hook fires exactly once per buffer whether it was opened directly or escalated from a preview. 2. Fewer, non-redundant states. Preview was tracked in three places that could drift: `Window.preview` (the anchored (split, buffer)), a per-buffer `BufferMetadata.is_preview` bool, and the `opening_as_preview` open-time flag. Collapsed to one source of truth: - `Window.preview: Option<(LeafId, BufferId)>` is now the only preview state. "Two previews", "orphan preview flag", and "flag disagrees with the anchored preview" are no longer representable. `is_buffer_preview` and the tab renderer derive from it (the renderer takes a `preview_buffer` argument threaded from `window.preview`). - `opening_as_preview` (ambient mutable flag) is replaced by an explicit `OpenKind::{Commit, Preview}` value threaded through the open path, so the hook-deferral intent travels as an argument rather than window state read out of band. Because the single `after_file_open` fire site keys deferral off `OpenKind` and re-fires off `Window.preview` at promotion, exactly-once holds structurally with no "hook already fired" bookkeeping: a directly-opened buffer is never the preview, and a promoted buffer is cleared from `Window.preview`, so a later promote is a no-op. The e2e suite adds the escalation case (preview → edit → hook fires) next to the existing preview-suppression case; it times out without the fire-on- promote change and passes with it. https://claude.ai/code/session_01KdY8xi7GwhBTJueszhwGXB
1 parent cc14104 commit 9c54f38

12 files changed

Lines changed: 227 additions & 122 deletions

File tree

crates/fresh-editor/src/app/buffer_management.rs

Lines changed: 22 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -137,16 +137,15 @@ impl Editor {
137137
// (LSP, language detection, split targeting, status message, plugin
138138
// hooks) consistent with a normal open.
139139
//
140-
// `opening_as_preview` additionally tells the inner open path that
141-
// this is a browse, not a deliberate open, so the `after_file_open`
142-
// plugin hook is skipped — otherwise plugins raise intrusive UI
143-
// (e.g. the asm-lsp `.asm-lsp.toml` config-offer popup) over each
144-
// file the user arrows past. Set and cleared around the call so a
145-
// failed open can't leave the flag stuck.
140+
// `OpenKind::Preview` additionally tells the open path that this is a
141+
// browse, not a deliberate open, so the `after_file_open` plugin hook
142+
// is deferred — otherwise plugins raise intrusive UI (e.g. the
143+
// asm-lsp `.asm-lsp.toml` config-offer popup) over each file the user
144+
// arrows past. The hook fires later if/when this preview is escalated
145+
// to a permanent tab.
146146
self.active_window_mut().suppress_position_history_once = true;
147-
self.active_window_mut().opening_as_preview = true;
148-
let open_result = self.open_file(path);
149-
self.active_window_mut().opening_as_preview = false;
147+
let open_result =
148+
self.open_file_with_kind(path, super::file_open_orchestrators::OpenKind::Preview);
150149
self.active_window_mut().suppress_position_history_once = false;
151150
let buffer_id = open_result?;
152151
let is_new = !previously_file_backed.contains(&buffer_id);
@@ -159,40 +158,37 @@ impl Editor {
159158
}
160159

161160
// New buffer. Resolve the existing preview (if any) relative to the
162-
// target split.
161+
// target split. `preview.take()` clears the single source of truth;
162+
// each arm below decides whether the displaced buffer was *committed*
163+
// (escalated → fire its deferred `after_file_open`) or merely
164+
// *discarded* (closed → no hook).
163165
match self.active_window_mut().preview.take() {
164166
Some((prev_split, old_id)) if prev_split == target_split => {
165167
// Same split: close the old preview so the new one takes its
166-
// place. If close fails (modified buffer — shouldn't happen
167-
// because edits promote, but defend in depth), demote the
168-
// orphan to a permanent tab rather than leaving behind an
169-
// italic "(preview)" tab that will never be replaced.
168+
// place. Replacement is not a commitment, so no hook. If close
169+
// fails (modified buffer — shouldn't happen because edits
170+
// promote, but defend in depth), the buffer stays open as a
171+
// permanent tab, which *is* a commitment → fire its deferred
172+
// hook.
170173
if let Err(e) = self.close_buffer(old_id) {
171174
tracing::warn!(
172175
"preview: could not replace stale preview buffer {:?}, demoting to permanent: {}",
173176
old_id,
174177
e
175178
);
176-
if let Some(m) = self.active_window_mut().buffer_metadata.get_mut(&old_id) {
177-
m.is_preview = false;
178-
}
179+
self.active_window().fire_deferred_after_file_open(old_id);
179180
}
180181
}
181182
Some((_other_split, old_id)) => {
182183
// Different split: user walked away from the old preview
183-
// before this click. Promote it to permanent — their focus
184-
// moving to another split was the commitment signal.
185-
if let Some(m) = self.active_window_mut().buffer_metadata.get_mut(&old_id) {
186-
m.is_preview = false;
187-
}
184+
// before this click. Their focus moving to another split was
185+
// the commitment signal → escalate it and fire the hook.
186+
self.active_window().fire_deferred_after_file_open(old_id);
188187
}
189188
None => {}
190189
}
191190

192-
// Mark the new buffer as the preview, anchored to its split.
193-
if let Some(meta) = self.active_window_mut().buffer_metadata.get_mut(&buffer_id) {
194-
meta.is_preview = true;
195-
}
191+
// Anchor the new buffer as the preview — the single source of truth.
196192
self.active_window_mut().preview = Some((target_split, buffer_id));
197193

198194
Ok(buffer_id)

crates/fresh-editor/src/app/file_open_orchestrators.rs

Lines changed: 82 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,24 @@ use crate::state::EditorState;
2121

2222
use super::Editor;
2323

24+
/// How a file open treats the resulting buffer.
25+
///
26+
/// Threaded through the open path so the single `after_file_open` fire site
27+
/// in [`open_file_no_focus_inner`] can defer the hook for previews. A
28+
/// `Preview` open is "just looking" (file-explorer browse, live-grep
29+
/// overlay): the hook is withheld until the preview is escalated to a
30+
/// permanent buffer (see the `promote_*` methods on `Window`). `Commit` is
31+
/// every deliberate open and fires the hook immediately. This replaces an
32+
/// earlier ambient `opening_as_preview` flag — the intent now travels as a
33+
/// value rather than mutable window state that could be read out of band.
34+
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
35+
pub(crate) enum OpenKind {
36+
/// Deliberate open — fire `after_file_open` now.
37+
Commit,
38+
/// Ephemeral preview — defer `after_file_open` until escalation.
39+
Preview,
40+
}
41+
2442
impl Editor {
2543
/// Helper to jump to a line/column position in the active buffer.
2644
///
@@ -98,6 +116,18 @@ impl Editor {
98116
/// If the file doesn't exist, creates an unsaved buffer with that filename.
99117
/// Saving the buffer will create the file.
100118
pub fn open_file(&mut self, path: &Path) -> anyhow::Result<BufferId> {
119+
self.open_file_with_kind(path, OpenKind::Commit)
120+
}
121+
122+
/// `open_file` with explicit [`OpenKind`]. `open_file_preview` uses
123+
/// `OpenKind::Preview` to route through all of `open_file`'s
124+
/// cross-cutting concerns (focus, language detection, status, the
125+
/// `buffer_activated` hook) while deferring only `after_file_open`.
126+
pub(crate) fn open_file_with_kind(
127+
&mut self,
128+
path: &Path,
129+
kind: OpenKind,
130+
) -> anyhow::Result<BufferId> {
101131
// If the active leaf is a utility-dock pane (Search/Replace,
102132
// Quickfix, terminal-in-dock), the user almost never wants the
103133
// newly-opened file to land there — the dock hosts panel-style
@@ -119,7 +149,9 @@ impl Editor {
119149
.and_then(|s| s.buffer.file_path())
120150
.is_some();
121151

122-
let buffer_id = self.active_window_mut().open_file_no_focus(path)?;
152+
let buffer_id = self
153+
.active_window_mut()
154+
.open_file_no_focus_with_kind(path, kind)?;
123155

124156
// Check if this was an already-open buffer or a new one
125157
// For already-open buffers, just switch to them
@@ -817,7 +849,18 @@ impl crate::app::window::Window {
817849
/// flip. If the file doesn't exist, creates an unsaved buffer with
818850
/// that filename.
819851
pub fn open_file_no_focus(&mut self, path: &Path) -> anyhow::Result<BufferId> {
820-
self.open_file_no_focus_inner(path, true)
852+
self.open_file_no_focus_inner(path, true, OpenKind::Commit)
853+
}
854+
855+
/// `open_file_no_focus` with an explicit [`OpenKind`], so the Editor-side
856+
/// `open_file_with_kind` can route a preview open through the same core
857+
/// while deferring `after_file_open`.
858+
pub(crate) fn open_file_no_focus_with_kind(
859+
&mut self,
860+
path: &Path,
861+
kind: OpenKind,
862+
) -> anyhow::Result<BufferId> {
863+
self.open_file_no_focus_inner(path, true, kind)
821864
}
822865

823866
/// Open a file without switching focus AND without ever
@@ -832,14 +875,10 @@ impl crate::app::window::Window {
832875
/// results. This variant always allocates a fresh BufferId so the
833876
/// background buffer never gets repurposed.
834877
pub(crate) fn open_file_for_preview(&mut self, path: &Path) -> anyhow::Result<BufferId> {
835-
// Live-grep preview is a browse, not a deliberate open: suppress the
836-
// `after_file_open` plugin hook so plugins don't pop UI / run side
837-
// effects over each result the user cycles through. Cleared
838-
// unconditionally so an error can't leave the flag stuck.
839-
self.opening_as_preview = true;
840-
let result = self.open_file_no_focus_inner(path, false);
841-
self.opening_as_preview = false;
842-
result
878+
// Live-grep preview is a browse, not a deliberate open: defer the
879+
// `after_file_open` hook so plugins don't pop UI / run side effects
880+
// over each result the user cycles through.
881+
self.open_file_no_focus_inner(path, false, OpenKind::Preview)
843882
}
844883

845884
/// True if `path` is an internal app artifact (terminal scrollback,
@@ -866,6 +905,7 @@ impl crate::app::window::Window {
866905
&mut self,
867906
path: &Path,
868907
allow_replace_empty: bool,
908+
kind: OpenKind,
869909
) -> anyhow::Result<BufferId> {
870910
// Fail fast if the remote connection is down — don't attempt I/O that
871911
// would either timeout or return confusing errors.
@@ -1172,20 +1212,40 @@ impl crate::app::window::Window {
11721212
// looking": firing this hook lets plugins raise intrusive UI (e.g.
11731213
// the asm-lsp config-offer popup) or run side effects (csharp
11741214
// `dotnet restore`) over a file the user is merely glancing at as
1175-
// previews replace each other. Deliberate opens (Ctrl+P, double-
1176-
// click, Enter) go through the non-preview path and still fire it.
1177-
// Plugins that need to react when a preview becomes visible use
1178-
// `buffer_activated`, which still fires on every preview switch.
1179-
if !self.opening_as_preview {
1180-
self.resources.plugin_manager.read().unwrap().run_hook(
1181-
"after_file_open",
1182-
crate::services::plugins::hooks::HookArgs::AfterFileOpen {
1183-
buffer_id,
1184-
path: path.to_path_buf(),
1185-
},
1186-
);
1215+
// previews replace each other. For a preview the hook is deferred —
1216+
// it fires once the buffer is escalated to a permanent tab (see
1217+
// `Window::promote_*`). Plugins that need to react when a preview
1218+
// becomes visible use `buffer_activated`, which still fires on every
1219+
// preview switch.
1220+
if kind == OpenKind::Commit {
1221+
self.run_after_file_open_hook(buffer_id, path.to_path_buf());
11871222
}
11881223

11891224
Ok(buffer_id)
11901225
}
1226+
1227+
/// Fire the `after_file_open` plugin hook for `buffer_id`. Single site so
1228+
/// both the commit-time open path and the escalation (promote) path raise
1229+
/// it identically.
1230+
pub(crate) fn run_after_file_open_hook(&self, buffer_id: BufferId, path: std::path::PathBuf) {
1231+
self.resources.plugin_manager.read().unwrap().run_hook(
1232+
"after_file_open",
1233+
crate::services::plugins::hooks::HookArgs::AfterFileOpen { buffer_id, path },
1234+
);
1235+
}
1236+
1237+
/// Fire the deferred `after_file_open` hook for a buffer that was opened
1238+
/// as a preview and is now being escalated to a permanent tab. Looks up
1239+
/// the buffer's own file path; a no-op for buffers without one.
1240+
pub(crate) fn fire_deferred_after_file_open(&self, buffer_id: BufferId) {
1241+
if let Some(path) = self
1242+
.buffers
1243+
.get(&buffer_id)
1244+
.and_then(|s| s.buffer.file_path())
1245+
.filter(|p| !p.as_os_str().is_empty())
1246+
.map(|p| p.to_path_buf())
1247+
{
1248+
self.run_after_file_open_hook(buffer_id, path);
1249+
}
1250+
}
11911251
}

crates/fresh-editor/src/app/macro_actions.rs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,6 @@ impl Editor {
245245
hidden_from_tabs: false,
246246
auto_revert_enabled: true,
247247
synthetic_placeholder: false,
248-
is_preview: false,
249248
recovery_id: None,
250249
};
251250
self.active_window_mut()
@@ -331,7 +330,6 @@ impl Editor {
331330
hidden_from_tabs: false,
332331
auto_revert_enabled: true,
333332
synthetic_placeholder: false,
334-
is_preview: false,
335333
recovery_id: None,
336334
};
337335
self.active_window_mut()

crates/fresh-editor/src/app/plugin_dispatch.rs

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5115,11 +5115,7 @@ impl Window {
51155115
.map(|bs| matches!(bs.view_mode, crate::state::ViewMode::PageView))
51165116
.unwrap_or(false)
51175117
});
5118-
let is_preview = self
5119-
.buffer_metadata
5120-
.get(buffer_id)
5121-
.map(|m| m.is_preview)
5122-
.unwrap_or(false);
5118+
let is_preview = self.is_buffer_preview(*buffer_id);
51235119
// Which splits currently hold this buffer — lets plugins
51245120
// implement "focus existing if visible, else open new"
51255121
// without tracking split ids across editor restarts

crates/fresh-editor/src/app/render.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -672,6 +672,10 @@ impl Editor {
672672
.get_mut(&active_window_id)
673673
.expect("active window must exist");
674674
let __metadata_ref = &__win.buffer_metadata;
675+
// Copy out the preview buffer id (the single source of truth) so the
676+
// tab renderer can style the "(preview)" tab without holding a borrow
677+
// of `__win` across the `with_all_mut` closure below.
678+
let __preview_buffer = __win.preview.map(|(_, b)| b);
675679
let __event_logs_mut = &mut __win.event_logs;
676680
let __grouped_ref = &__win.grouped_subtrees;
677681
let __composite_buffers_mut = &mut __win.composite_buffers;
@@ -695,6 +699,7 @@ impl Editor {
695699
&*__mgr,
696700
__buffers_mut,
697701
__metadata_ref,
702+
__preview_buffer,
698703
__event_logs_mut,
699704
__composite_buffers_mut,
700705
__composite_view_states_mut,
@@ -2250,6 +2255,7 @@ impl Editor {
22502255
return;
22512256
};
22522257
let __preview_metadata = &__win_for_preview.buffer_metadata;
2258+
let __preview_buffer_id = __win_for_preview.preview.map(|(_, b)| b);
22532259
let __preview_event_logs = &mut __win_for_preview.event_logs;
22542260
let __preview_composite_buffers = &mut __win_for_preview.composite_buffers;
22552261
let __preview_composite_view_states = &mut __win_for_preview.composite_view_states;
@@ -2287,6 +2293,7 @@ impl Editor {
22872293
&*mgr,
22882294
preview_buffers,
22892295
__preview_metadata,
2296+
__preview_buffer_id,
22902297
__preview_event_logs,
22912298
__preview_composite_buffers,
22922299
__preview_composite_view_states,

crates/fresh-editor/src/app/scroll_sync.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ impl crate::app::window::Window {
4040
.collect();
4141
let metadata = &self.buffer_metadata;
4242
let composites = &self.composite_buffers;
43+
let preview_buffer = self.preview.map(|(_, b)| b);
4344

4445
self.buffers.with_all_mut(|buffer_map, _mgr, vs_map| {
4546
let Some(view_state) = vs_map.get_mut(&split_id) else {
@@ -52,6 +53,7 @@ impl crate::app::window::Window {
5253
metadata,
5354
composites,
5455
&group_names,
56+
preview_buffer,
5557
);
5658

5759
let total_tabs_width: usize = tab_widths.iter().sum();

crates/fresh-editor/src/app/types/buffer_meta.rs

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -71,19 +71,6 @@ pub struct BufferMetadata {
7171
/// user a truly empty workspace.
7272
pub synthetic_placeholder: bool,
7373

74-
/// Whether this buffer is opened in "preview" mode (ephemeral).
75-
/// A preview buffer is one opened by a single-click in the file explorer
76-
/// (or a similar soft-open gesture). Its tab is rendered in italic and
77-
/// it is replaced the next time another file is opened the same way.
78-
/// The flag is cleared ("promoted") when the user edits the buffer,
79-
/// double-clicks the file, or otherwise signals commitment to the file.
80-
///
81-
/// Intentionally ephemeral — never serialized into workspace or
82-
/// recovery state. Restarting the editor always brings buffers back
83-
/// as permanent tabs; preview status belongs to the current session's
84-
/// exploration flow only.
85-
pub is_preview: bool,
86-
8774
/// Stable recovery ID for unnamed buffers.
8875
/// For file-backed buffers, recovery ID is computed from the path hash.
8976
/// For unnamed buffers, this is generated once and reused across auto-saves.
@@ -150,7 +137,6 @@ impl BufferMetadata {
150137
hidden_from_tabs: false,
151138
auto_revert_enabled: true,
152139
synthetic_placeholder: false,
153-
is_preview: false,
154140
recovery_id: None,
155141
}
156142
}
@@ -172,7 +158,6 @@ impl BufferMetadata {
172158
auto_revert_enabled: true,
173159
hidden_from_tabs: false,
174160
synthetic_placeholder: false,
175-
is_preview: false,
176161
recovery_id: None,
177162
}
178163
}
@@ -233,7 +218,6 @@ impl BufferMetadata {
233218
lsp_opened_with: HashSet::new(),
234219
hidden_from_tabs: false,
235220
synthetic_placeholder: false,
236-
is_preview: false,
237221
recovery_id: None,
238222
}
239223
}
@@ -273,7 +257,6 @@ impl BufferMetadata {
273257
lsp_opened_with: HashSet::new(),
274258
hidden_from_tabs: false,
275259
synthetic_placeholder: false,
276-
is_preview: false,
277260
recovery_id: None,
278261
}
279262
}
@@ -372,7 +355,6 @@ impl BufferMetadata {
372355
lsp_opened_with: HashSet::new(),
373356
hidden_from_tabs: false,
374357
synthetic_placeholder: false,
375-
is_preview: false,
376358
recovery_id: None,
377359
}
378360
}
@@ -392,7 +374,6 @@ impl BufferMetadata {
392374
lsp_opened_with: HashSet::new(),
393375
hidden_from_tabs: true,
394376
synthetic_placeholder: false,
395-
is_preview: false,
396377
recovery_id: None,
397378
}
398379
}

0 commit comments

Comments
 (0)