Skip to content

Commit 959ddd1

Browse files
jkyberneeesclaude
andcommitted
Address second review: tab bar state, relaunch order, menu keys, marker race
macOS shell: - The tab bar offset comes from tabGroup.isTabBarVisible (KVO, re-attached when the window changes group). In full screen it uses the height measured in a window (28pt fallback), so a toolbar reveal never shifts the page. - didCommit also re-sends data-fullscreen, so a reload in full screen stays right. - Relaunch writes each group in visual tab order (tabbedWindows). - Without --window (the CLI, or a restart from an older build) every --folder tabs as before, so old separate windows aren't merged. - New Window with several folders: one new window holding them all as tabs. Client: - App shortcuts step aside while the context menu is open (isContextMenuOpen). - Change markers are pruned only when git takes a path from changed to clean or ignored, not when a stale status merely omits it (GitState.cleared, tested). - The explorer always suppresses the WebView menu: folders get Expand/Collapse and Collapse All, empty space gets Collapse All. - The context menu and the diff peek share one dismissable() helper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent c46395e commit 959ddd1

8 files changed

Lines changed: 140 additions & 76 deletions

File tree

‎packaging/macos/main.swift‎

Lines changed: 39 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,8 @@ final class ViewerWindow: NSObject, NSWindowDelegate, WKNavigationDelegate, WKUI
106106
private var tabbarHeight = -1 // last value sent to the page
107107
private var measuredTabbar = 0 // AppKit's tab bar height, measured outside full screen
108108
private var layoutObserver: NSKeyValueObservation?
109+
private var tabBarObserver: NSKeyValueObservation?
110+
private weak var observedGroup: NSWindowTabGroup?
109111

110112
init(folder: URL) {
111113
server = Server(folder: folder)
@@ -166,17 +168,23 @@ final class ViewerWindow: NSObject, NSWindowDelegate, WKNavigationDelegate, WKUI
166168

167169
/// Tells the page how tall the native tab bar is (0 when hidden).
168170
func syncTabBar(force: Bool = false) {
169-
let inset = ViewerWindow.titlebarMetrics(window).height
170-
var h: Int
171-
if window.styleMask.contains(.fullScreen) {
172-
// The title bar is gone; a tab bar that stays on screen is the whole inset. A larger
173-
// inset is the toolbar sliding in while the pointer is at the top edge: an overlay,
174-
// so keep the current offset rather than shifting the page under the pointer.
175-
if inset > max(measuredTabbar, 1) + 4 { return }
176-
h = inset
177-
} else {
178-
h = max(0, inset - titlebarHeight)
179-
if h > 0 { measuredTabbar = h }
171+
// The window moves to another group when its tab is dragged out or into another window.
172+
if window.tabGroup !== observedGroup {
173+
observedGroup = window.tabGroup
174+
tabBarObserver = observedGroup?.observe(\.isTabBarVisible) { [weak self] _, _ in
175+
DispatchQueue.main.async { self?.syncTabBar() }
176+
}
177+
}
178+
var h = 0
179+
if window.tabGroup?.isTabBarVisible == true {
180+
if window.styleMask.contains(.fullScreen) {
181+
// The title bar is gone, and the inset also grows while the toolbar slides in at the
182+
// top edge (an overlay): use the height measured in a window, not the inset.
183+
h = measuredTabbar > 0 ? measuredTabbar : 28
184+
} else {
185+
h = max(0, ViewerWindow.titlebarMetrics(window).height - titlebarHeight)
186+
if h > 0 { measuredTabbar = h }
187+
}
180188
}
181189
if h == tabbarHeight && !force { return }
182190
tabbarHeight = h
@@ -260,9 +268,10 @@ final class ViewerWindow: NSObject, NSWindowDelegate, WKNavigationDelegate, WKUI
260268
}
261269
}
262270

263-
// A new document starts without the offset: send it before the page first paints.
271+
// A new document starts without the offset or full screen state: send them before the page first paints.
264272
func webView(_ webView: WKWebView, didCommit navigation: WKNavigation!) {
265273
syncTabBar(force: true)
274+
setFullScreenAttribute(window.styleMask.contains(.fullScreen))
266275
}
267276

268277
func webView(_ webView: WKWebView, didFinish navigation: WKNavigation!) {
@@ -311,6 +320,7 @@ final class ViewerWindow: NSObject, NSWindowDelegate, WKNavigationDelegate, WKUI
311320

312321
func windowWillClose(_ notification: Notification) {
313322
layoutObserver = nil
323+
tabBarObserver = nil
314324
webView.configuration.userContentController.removeScriptMessageHandler(forName: "native")
315325
server.stop()
316326
onClose?(self)
@@ -340,11 +350,14 @@ final class AppDelegate: NSObject, NSApplicationDelegate {
340350
func applicationDidFinishLaunching(_ notification: Notification) {
341351
// `codeviewer <dir>` from a terminal runs: open -n -a Codeviewer --args --folder <dir>
342352
// (a restart after an update passes one --folder per tab, --window to start a new window).
353+
// Without --window (the CLI, or a restart from an older build) each --folder tabs as usual.
343354
let args = CommandLine.arguments
355+
let layout = args.contains("--window")
344356
var last: ViewerWindow?
345357
for (i, a) in args.enumerated() where (a == "--folder" || a == "--window") && i + 1 < args.count {
358+
let url = URL(fileURLWithPath: args[i + 1], isDirectory: true)
346359
let tabbed = a == "--folder"
347-
last = open(folder: URL(fileURLWithPath: args[i + 1], isDirectory: true), tabbed: tabbed, into: tabbed ? last?.window : nil)
360+
last = layout ? open(folder: url, tabbed: tabbed, into: tabbed ? last?.window : nil) : open(folder: url)
348361
}
349362
DispatchQueue.main.asyncAfter(deadline: .now() + 0.3) {
350363
if !self.openedSomething { self.chooseFolder(nil) }
@@ -420,7 +433,12 @@ final class AppDelegate: NSObject, NSApplicationDelegate {
420433
panel.message = "Choose a folder to view"
421434
NSApp.activate(ignoringOtherApps: true)
422435
if panel.runModal() == .OK {
423-
panel.urls.forEach { open(folder: $0, tabbed: tabbed) }
436+
// New Window with several folders: one new window, the rest as its tabs.
437+
var host: NSWindow?
438+
for u in panel.urls {
439+
let w = open(folder: u, tabbed: tabbed || host != nil, into: host)
440+
if !tabbed && host == nil { host = w.window }
441+
}
424442
} else if windows.isEmpty {
425443
NSApp.terminate(nil)
426444
}
@@ -430,17 +448,15 @@ final class AppDelegate: NSObject, NSApplicationDelegate {
430448
/// window layout: tabs of one window stay together, separate windows stay separate.
431449
func relaunch() {
432450
var args = ["-n", Bundle.main.bundlePath, "--args"]
433-
var groups: [[ViewerWindow]] = []
434-
for w in windows {
435-
if let i = groups.firstIndex(where: { $0[0].window.tabbedWindows?.contains(w.window) == true }) {
436-
groups[i].append(w)
437-
} else {
438-
groups.append([w])
451+
var done = Set<ObjectIdentifier>()
452+
for w in windows where !done.contains(ObjectIdentifier(w)) {
453+
// tabbedWindows is in tab order; nil for a window that stands alone.
454+
let group = (w.window.tabbedWindows ?? [w.window]).compactMap { nsw in windows.first { $0.window === nsw } }
455+
for (i, t) in group.enumerated() {
456+
done.insert(ObjectIdentifier(t))
457+
args += [i == 0 ? "--window" : "--folder", t.server.folder.path]
439458
}
440459
}
441-
for g in groups {
442-
for (i, w) in g.enumerated() { args += [i == 0 ? "--window" : "--folder", w.server.folder.path] }
443-
}
444460
let p = Process()
445461
p.executableURL = URL(fileURLWithPath: "/usr/bin/open")
446462
p.arguments = args

‎src/client/app.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { icons } from "./icons.ts";
99
import { GitState, relativeTime } from "./git.ts";
1010
import { Updates } from "./updates.ts";
1111
import { debounce, dragSplitter, esc, formatBytes, h, IS_MAC, MOD, store } from "./util.ts";
12+
import { isContextMenuOpen } from "./menu.ts";
1213

1314
type ThemeName = "dark" | "light" | "system";
1415

@@ -531,6 +532,7 @@ class App {
531532
window.addEventListener(
532533
"keydown",
533534
(e) => {
535+
if (isContextMenuOpen()) return; // its own listener takes the key
534536
const mod = IS_MAC ? e.metaKey : e.ctrlKey;
535537
const k = e.key.toLowerCase();
536538
const inTerm = (e.target as HTMLElement).classList?.contains("term-input");

‎src/client/codeview.ts‎

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { Highlighter, type Tok } from "./highlight.ts";
2-
import { esc, h } from "./util.ts";
2+
import { dismissable, esc, h } from "./util.ts";
33
import { icons } from "./icons.ts";
44
import { diffLines, gutterMarks, ADDED, MODIFIED, DELETED_BELOW, DELETED_ABOVE, type Hunk } from "./diff.ts";
55

@@ -254,18 +254,9 @@ export class CodeView {
254254
box.style.left = `${this.gutterW - 6}px`;
255255
this.el.append(box);
256256
this.peek = box;
257-
const onKey = (e: KeyboardEvent) => {
258-
if (e.key === "Escape") this.closePeek();
259-
};
260-
const onDown = (e: MouseEvent) => {
261-
if (!box.contains(e.target as Node) && !(e.target as HTMLElement).classList.contains("g")) this.closePeek();
262-
};
263-
window.addEventListener("keydown", onKey, true);
264-
window.addEventListener("mousedown", onDown, true);
265-
box.addEventListener("peek-close", () => {
266-
window.removeEventListener("keydown", onKey, true);
267-
window.removeEventListener("mousedown", onDown, true);
268-
});
257+
// A click on a gutter marker toggles the peek itself.
258+
const off = dismissable(box, () => this.closePeek(), { keep: (t) => !!t.classList?.contains("g") });
259+
box.addEventListener("peek-close", off);
269260
}
270261

271262
private closePeek() {

‎src/client/git.ts‎

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -52,14 +52,16 @@ export class GitState {
5252
status: GitStatus = { repo: false } as GitStatus;
5353
private files = new Map<string, FileGit>();
5454
private prefixes = new Map<string, FileGit>(); // untracked/ignored directories (no trailing "/")
55+
private prev = { files: this.files, prefixes: this.prefixes };
5556
private dirs = new Map<string, Letter>();
5657
private listeners = new Set<() => void>();
5758
counts = { changed: 0, staged: 0, untracked: 0, conflicts: 0 };
5859

5960
set(s: GitStatus) {
6061
this.status = s;
61-
this.files.clear();
62-
this.prefixes.clear();
62+
this.prev = { files: this.files, prefixes: this.prefixes };
63+
this.files = new Map();
64+
this.prefixes = new Map();
6365
this.dirs.clear();
6466
this.counts = { changed: 0, staged: 0, untracked: 0, conflicts: 0 };
6567
for (const [raw, xy] of Object.entries(s.files ?? {})) {
@@ -93,23 +95,20 @@ export class GitState {
9395

9496
file(path: string): FileGit | null {
9597
path = nfc(path);
96-
return this.files.get(path) ?? this.underPrefix(path);
98+
return this.files.get(path) ?? underPrefix(this.prefixes, path);
9799
}
98100

99-
/** Status of a directory: its own (untracked/ignored) or the strongest inside it. */
100-
dir(path: string): Letter | null {
101+
/** True if the last status update took `path` from changed to clean or ignored. */
102+
cleared(path: string): boolean {
101103
path = nfc(path);
102-
return (this.prefixes.get(path) ?? this.underPrefix(path))?.letter ?? this.dirs.get(path) ?? null;
104+
const was = this.prev.files.get(path) ?? underPrefix(this.prev.prefixes, path);
105+
return !!was && was.letter !== "I" && (this.file(path)?.letter ?? "I") === "I";
103106
}
104107

105-
/** Nearest untracked/ignored ancestor directory, walking up (O(depth)). */
106-
private underPrefix(path: string): FileGit | null {
107-
if (!this.prefixes.size) return null;
108-
for (let i = path.lastIndexOf("/"); i > 0; i = path.lastIndexOf("/", i - 1)) {
109-
const g = this.prefixes.get(path.slice(0, i));
110-
if (g) return g;
111-
}
112-
return null;
108+
/** Status of a directory: its own (untracked/ignored) or the strongest inside it. */
109+
dir(path: string): Letter | null {
110+
path = nfc(path);
111+
return (this.prefixes.get(path) ?? underPrefix(this.prefixes, path))?.letter ?? this.dirs.get(path) ?? null;
113112
}
114113

115114
/** Changed (non-ignored) files, most important first. */
@@ -121,6 +120,16 @@ export class GitState {
121120
}
122121
}
123122

123+
/** Nearest untracked/ignored ancestor directory, walking up (O(depth)). */
124+
function underPrefix(prefixes: Map<string, FileGit>, path: string): FileGit | null {
125+
if (!prefixes.size) return null;
126+
for (let i = path.lastIndexOf("/"); i > 0; i = path.lastIndexOf("/", i - 1)) {
127+
const g = prefixes.get(path.slice(0, i));
128+
if (g) return g;
129+
}
130+
return null;
131+
}
132+
124133
export function relativeTime(ms: number): string {
125134
const s = Math.round((Date.now() - ms) / 1000);
126135
const units: [number, string][] = [

‎src/client/menu.ts‎

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { h } from "./util.ts";
1+
import { dismissable, h } from "./util.ts";
22

33
export interface MenuItem {
44
label: string;
@@ -11,6 +11,10 @@ export function closeContextMenu() {
1111
current?.dispose();
1212
}
1313

14+
export function isContextMenuOpen() {
15+
return !!current;
16+
}
17+
1418
/** Shows a small context menu at the pointer, replacing the browser's (or WebView's) own. */
1519
export function showContextMenu(e: MouseEvent, items: MenuItem[]) {
1620
e.preventDefault();
@@ -39,10 +43,7 @@ export function showContextMenu(e: MouseEvent, items: MenuItem[]) {
3943
el.style.top = `${Math.max(4, Math.min(e.clientY, innerHeight - r.height - 4))}px`;
4044

4145
const buttons = [...el.querySelectorAll<HTMLElement>(".ctx-item")];
42-
const onDown = (ev: Event) => {
43-
if (!el.contains(ev.target as Node)) closeContextMenu();
44-
};
45-
// While open, the menu owns the keyboard: no shortcut runs underneath it.
46+
// While open, the menu owns the keyboard: App's shortcuts check isContextMenuOpen() and step aside.
4647
const onKey = (ev: KeyboardEvent) => {
4748
ev.stopPropagation();
4849
if (["Shift", "Control", "Alt", "Meta"].includes(ev.key)) return;
@@ -56,22 +57,12 @@ export function showContextMenu(e: MouseEvent, items: MenuItem[]) {
5657
} else closeContextMenu();
5758
ev.preventDefault();
5859
};
59-
const onBlur = () => closeContextMenu();
60-
// Capture phase, so a click elsewhere closes the menu even if a handler stops propagation.
61-
window.addEventListener("mousedown", onDown, true);
62-
window.addEventListener("keydown", onKey, true);
63-
window.addEventListener("blur", onBlur);
64-
window.addEventListener("resize", onBlur);
65-
window.addEventListener("wheel", onBlur, { passive: true, capture: true });
60+
const off = dismissable(el, closeContextMenu, { key: onKey, transient: true });
6661
current = {
6762
dispose: () => {
6863
current = null;
6964
el.remove();
70-
window.removeEventListener("mousedown", onDown, true);
71-
window.removeEventListener("keydown", onKey, true);
72-
window.removeEventListener("blur", onBlur);
73-
window.removeEventListener("resize", onBlur);
74-
window.removeEventListener("wheel", onBlur, true);
65+
off();
7566
// Only if focus was still in the menu (or lost with it); a click elsewhere keeps its target.
7667
if (returnTo?.isConnected && (!document.activeElement || document.activeElement === document.body)) {
7768
returnTo.focus({ preventScroll: true });

‎src/client/tree.ts‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ export class FileTree {
4545
new ResizeObserver(() => this.schedule()).observe(this.scroller);
4646
this.rows.addEventListener("click", (e) => this.onClick(e as MouseEvent, false));
4747
this.rows.addEventListener("dblclick", (e) => this.onClick(e as MouseEvent, true));
48-
this.rows.addEventListener("contextmenu", (e) => this.onContextMenu(e as MouseEvent));
48+
this.scroller.addEventListener("contextmenu", (e) => this.onContextMenu(e as MouseEvent));
4949
this.scroller.addEventListener("keydown", (e) => this.onKey(e));
5050
}
5151

@@ -182,15 +182,20 @@ export class FileTree {
182182
this.schedule();
183183
}
184184

185-
/** Our own menu instead of the WebView's (Reload, Inspect Element…). */
185+
/** Our own menu instead of the WebView's (Reload, Inspect Element…), anywhere in the explorer. */
186186
private onContextMenu(e: MouseEvent) {
187187
e.preventDefault();
188188
const row = (e.target as HTMLElement).closest<HTMLElement>(".row");
189189
const n = row && this.flat[Number(row.dataset.i)];
190-
if (!n || n.type !== "file") return;
190+
const collapse = { label: "Collapse All", run: () => this.collapseAll() };
191+
if (!n) return showContextMenu(e, [collapse]);
191192
this.selected = n.path;
192193
this.schedule();
193-
showContextMenu(e, [{ label: "Open in New Tab", run: () => this.onOpen(n.path, true) }]);
194+
if (n.type === "dir") {
195+
showContextMenu(e, [{ label: n.expanded ? "Collapse" : "Expand", run: () => void this.toggle(n) }, collapse]);
196+
} else if (n.type === "file") {
197+
showContextMenu(e, [{ label: "Open in New Tab", run: () => this.onOpen(n.path, true) }]);
198+
}
194199
}
195200

196201
private async toggle(n: TNode, open = !n.expanded) {
@@ -332,14 +337,15 @@ export class FileTree {
332337
this.reflow();
333338
}
334339

335-
/** Git status changed: committed, reverted or ignored files no longer count as changes. */
340+
/**
341+
* Git status changed: files it just saw committed, reverted or ignored no longer count as
342+
* changes. Only that transition prunes: a status taken before a write would still call the
343+
* file clean, and the next one (showing it modified) wouldn't bring the marker back.
344+
*/
336345
onGitChanged() {
337346
if (this.git?.repo) {
338347
let pruned = false;
339-
for (const p of this.changed) {
340-
const g = this.git.file(p);
341-
if (!g || g.letter === "I") pruned = this.changed.delete(p) || pruned;
342-
}
348+
for (const p of this.changed) if (this.git.cleared(p)) pruned = this.changed.delete(p) || pruned;
343349
if (pruned) this.recomputeChangedDirs();
344350
}
345351
this.schedule();

‎src/client/util.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,3 +126,35 @@ export function dragSplitter(handle: HTMLElement, axis: "x" | "y", onStart: () =
126126
handle.addEventListener("pointercancel", up);
127127
});
128128
}
129+
130+
/**
131+
* Closes a popup on a mousedown outside it and on Escape (or `key`, which then decides),
132+
* optionally also when the window blurs, resizes or scrolls. Capture phase, so handlers that
133+
* stop propagation can't keep it open. Returns the function that removes the listeners.
134+
*/
135+
export function dismissable(
136+
el: HTMLElement,
137+
onClose: () => void,
138+
opts: { key?: (e: KeyboardEvent) => void; keep?: (target: HTMLElement) => boolean; transient?: boolean } = {},
139+
): () => void {
140+
const onDown = (e: Event) => {
141+
const t = e.target as HTMLElement;
142+
if (!el.contains(t) && !opts.keep?.(t)) onClose();
143+
};
144+
const onKey = opts.key ?? ((e: KeyboardEvent) => e.key === "Escape" && onClose());
145+
const onAway = () => onClose();
146+
window.addEventListener("mousedown", onDown, true);
147+
window.addEventListener("keydown", onKey, true);
148+
if (opts.transient) {
149+
window.addEventListener("blur", onAway);
150+
window.addEventListener("resize", onAway);
151+
window.addEventListener("wheel", onAway, { passive: true, capture: true });
152+
}
153+
return () => {
154+
window.removeEventListener("mousedown", onDown, true);
155+
window.removeEventListener("keydown", onKey, true);
156+
window.removeEventListener("blur", onAway);
157+
window.removeEventListener("resize", onAway);
158+
window.removeEventListener("wheel", onAway, true);
159+
};
160+
}

‎test/gitstate.test.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,3 +31,20 @@ test("lookups stay fast with thousands of ignored folders", () => {
3131
for (let i = 0; i < 6000; i++) g.file(`pkg${i % 500}/src/module${i}.py`);
3232
expect(performance.now() - t).toBeLessThan(50 * BUDGET);
3333
});
34+
35+
test("cleared: only a changed-to-clean/ignored transition counts", () => {
36+
const g = new GitState();
37+
g.set({ ...base, files: { "a.ts": ".M", "b.ts": "??", "gen/": "!!", "c.ts": ".M", "out/": "??" } });
38+
expect(g.cleared("a.ts")).toBe(false); // nothing before this status
39+
// A status taken before d.ts was written still calls it clean: that mustn't count.
40+
g.set({ ...base, files: { "b.ts": "!!", "gen/": "!!", "c.ts": ".M" } });
41+
expect(g.cleared("a.ts")).toBe(true); // committed or reverted
42+
expect(g.cleared("b.ts")).toBe(true); // now ignored
43+
expect(g.cleared("out/x.js")).toBe(true); // untracked folder gone
44+
expect(g.cleared("c.ts")).toBe(false); // still modified
45+
expect(g.cleared("d.ts")).toBe(false); // clean before and after
46+
expect(g.cleared("gen/x.js")).toBe(false); // ignored before and after
47+
g.set({ ...base, files: { "c.ts": ".M", "d.ts": ".M" } });
48+
expect(g.cleared("d.ts")).toBe(false); // clean to modified
49+
expect(g.cleared("a.ts")).toBe(false); // clean to clean
50+
});

0 commit comments

Comments
 (0)