fix(tabs): fix tab transfer and window borders - #452
Merged
Conversation
This was referenced Aug 4, 2026
PathGao
added a commit
that referenced
this pull request
Aug 4, 2026
…453) #366 gated claim/complete/cancel on the caller being window-<token>. That label exists only for a window create_transfer_window just built, so offer_tab_to_window's destination -- an already-open window labelled main or window-<some other token> -- was refused 100% of the time. #452 fixed it by recording the target on the pending entry and accepting either label. It reached master, not a release. Nothing in the suite failed. The Rust test asserted the predicate behaves as written, and it did: the gate was self-consistently wrong. The script test asserted that the invoke('offer_tab_to_window', ...) call appears in the source, and it still appeared. Both confirmed an implementation exists and is internally consistent, which is precisely what a reachable path becoming unreachable leaves undisturbed. Four tests now drive the broker from stage to complete: * a transfer to an existing window -- stage from main, record the target the way offer_tab_to_window does, claim as that window, complete. This is the path that was dead. * a transfer to a new window -- claim as window-<token> with no target recorded, so the fix cannot trade one path for the other. * a bystander window refused at claim, complete and cancel, with the transfer left intact for its real destination afterwards. * the recorded target releasing its own claim, the only rollback a claimed transfer has: the source's timeout is deliberately inert once a claim exists, so a refusal here would strand the tab in both windows. The authorisation decision moves out of the three #[tauri::command] bodies onto the broker as claim_as/complete_as/cancel_as, which take the caller's label as &str; each command is now one line passing window.label(). A unit test cannot build a tauri::Window, so the gate was otherwise reachable only through the predicate it was written against. Same peek, same order, same error strings. Reverting the recorded-target arm of is_destination_authority and restoring the two predicate tests to their #366 wording leaves the suite at 3 failed / 151 passed -- all three of them these tests. Co-authored-by: PathGao <gaoyanbo@gaoyanbodeMacBook-Air.local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
PathGao
added a commit
that referenced
this pull request
Aug 4, 2026
#454) * test(scripts): read every source file through one normalizing helper Fifteen test files were red on a Windows checkout while v2.7.0 was being cut, and were hand-patched assertion by assertion in #452. The cause is two decisions made in the wrong place. Git on Windows defaults to `core.autocrlf=true`, so the whole working tree is checked out CRLF. Every test here that reads `src/` as text then matches it against a pattern containing a literal `\n`, or slices it with an anchor containing one — and none of those match the bytes on disk. `sliceBetween` turns that into `expected to find "…"` for an anchor that is right there in the file. The patch that landed fixed the assertions: `[^\r\n]` in one place, `source.includes('\r\n') ? '\r\n\t…' : '\n\t…'` in another. Correct, and 33 characters spread over 15 files that nothing requires the next test to repeat. The decision belongs on the read, not on each of the 135 places that consume it. export function readSource(path: string | URL): string { return readFileSync(path, 'utf8').replace(/\r\n/g, '\n'); } `string | URL` because both spellings were already in use — a cwd-relative string and `new URL('../src/…', import.meta.url)` — so no call site had to change shape to get normalized. So this replaces the ad-hoc `readFileSync(…, 'utf8')` in 57 files and *removes* the `\r?` workarounds that become redundant: the assertions go back to reading as `\n`, which is the form the files have in the repository. What is deliberately left reading raw bytes: the CRLF fixtures in frontMatter.test.ts, frontMatterProseBlock.test.ts and pasteUrlContext.test.ts. Those assert on the parser's handling of a CRLF *document* and each spells the document as a literal in the test, which is the right way round — a fixture whose bytes are the point must not depend on what Git checked out. Measured, on a working tree converted to CRLF end to end: with `readSource` reduced to a plain read, 21 tests across 8 files fail — character for character the same set that fails on master with #452's patch reverse-applied. With the normalization in place, 596/596 pass on that same CRLF tree, with the workarounds deleted rather than kept. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * test(scripts): fold two private directory walkers back into walkSourceFiles sourceTree.ts exists because three copies of `walk()` used to live in singleImplementationConvention, renderPipelineConvention and previewSanitize. Two more had survived that consolidation, and both paid for it in #452. i18nCoverage.test.ts built its paths with `join(dir, entry)`, which on Windows spells them with backslashes, and then compared them against forward-slash literals — so it needed the same `.replace(/\\/g, '/')` that `walkSourceFiles` has had all along. monacoStartupGraph.test.ts's copy escaped only because it happened to build paths with a template literal instead of `join`; nothing in it made that a decision rather than an accident. Both filters were already what `walkSourceFiles` matches. i18nCoverage's was `.svelte` or `.ts` against `walkSourceFiles`'s `.ts|.svelte|.js`, which is the same set today (`src/` contains no `.js`) and the wider one is the one that should win: a `.js` file calling `t()` is a file whose keys must resolve. monacoStartupGraph's was `/\.(svelte|ts|js)$/` — identical. Neither needed `walkSourceFiles` widened with an argument. monacoStartupGraph's local reader was itself named `readSource` and held its own `readFileSync`, which is how the file came to own a private copy of the line-ending decision. It is a *filter* on top of a read — only `<script>` blocks can carry an import — so it is now `importableSource` and delegates. Not folded: the `.replace(/\\/g, '/')` #452 added to `relative(process.cwd(), f)` in the same file. That normalizes the output of the *import-graph* walk, which resolves absolute paths and never goes through `walkSourceFiles`, so it is a real separator fix rather than a symptom of the duplication. It stays. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * test(scripts): guard the single reader and walker, and pin checkouts to LF A cleanup without a guard comes back. The two commits before this one removed every private read and every private walk; nothing yet stops the next test from adding one, and the loss is silent until someone runs the suite on Windows. singleImplementationConvention.test.ts already is the place a convention like this is written down: a row in RULES naming a marker and the complete set of files allowed to contain it. The only thing it could not express was a rule about the suite itself, because SOURCES was fixed to `src`. Rules now carry an optional `dir` and the tree walk is memoized per directory, so the two new rows scan `scripts` and everything else is unchanged. readFileSync( — allowed in scripts/sourceTree.ts readdirSync( — allowed in scripts/sourceTree.ts Both markers are the *call*, not the import: `import { readFileSync }` on its own reads nothing, and a file may legitimately keep the import for another member. Both failure messages name the replacement and how to call it rather than only reporting that something is wrong. First run, before either cleanup: 59 files flagged by the read rule and 2 by the walk rule. `every rule keeps at least one live implementation` covers the new rows too, so a marker that stops matching sourceTree.ts is reported instead of silently guarding nothing. .gitattributes is the belt to `readSource`'s braces, and it is the weaker half of the pair on purpose: it does not touch a tree that is already checked out — that needs `git add --renormalize .` — and it cannot stop an editor from writing CRLF into a file it saves. Read-side normalization is the half that actually holds; this just stops the tree arriving wrong in the first place. `* text=auto eol=lf`, with `.nsi/.nsh/.ps1/.bat/.cmd` pinned to CRLF. Those are consumed by NSIS and Chocolatey on Windows, and pinning them keeps the bytes those toolchains see identical to what a `core.autocrlf=true` runner hands them today — this is not the commit to also change the installer build. Every tracked file in the repo is LF right now, verified, so `git add --renormalize .` stages nothing outside this branch's own edits: no stored bytes change and no Linux or macOS working tree changes. What changes is the next Windows clone. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
PathGao
added a commit
that referenced
this pull request
Aug 5, 2026
) * fix(titlebar): let the window-tag editor be dismissed without saving `tagEditorOpen` went false in exactly two places: `applyTag` (Save, and Enter) and `clearTag` (Remove Tag). `clearTag`'s button renders under `{#if tabManager.windowTag}`, so on a window with no tag yet — the state the popover is most often opened in, from Home > Set window tag — Save was the only control that closed it. Escape did nothing, a click elsewhere did nothing, `togglePinnedTag` left it open, and the chip was not a toggle: `openTagEditor` set `tagEditorOpen = true` unconditionally. Clearing the field and pressing Save happens to close it, because `applyTag` then calls `setWindowTag(null)`, but nothing says so. This was an omission rather than a decision, and the evidence is in the same component: the Home menu, the theme menu and the kebab menu each take `Escape` on the container element, each appears in `handleGlobalDismiss`, and each has a trigger that toggles and calls `stopPropagation`. The tag editor was the only one of the four popovers missing from all three. So it now follows them rather than inventing a fourth pattern: * `Escape` on the `.tag-editor` container, alongside the `Enter` case that was already there. * `tagEditorOpen = false` in `handleGlobalDismiss`, and `tagEditorOpen` added to the `$effect` guard, which is what installs that function on window `click`, `contextmenu` and `blur`. The popover gets an `onclick` that stops propagation for the same reason the three menus have one — otherwise picking a colour would dismiss the editor. * the chip toggles instead of re-opening, and stops propagation. That second part is load-bearing: the effect installs the window listener before the opening click has finished bubbling, so without it the chip would open the popover into its own dismissal. All three menu triggers already do this. Dismissing DISCARDS the draft name and colour; only Save and Enter write a tag. Checked rather than assumed, and the sources disagree, so the reasoning is here. The element is `role="dialog"`, and the WAI-ARIA APG dialog pattern is "Escape: Closes the dialog", with Escape the standard mapping for Cancel on both desktop platforms. Apple's HIG points the other way for popovers — save on automatic close, discard only via an explicit Cancel — but that guidance is written for popovers that edit live and have no commit control, where closing is the only moment a value could be kept. This one has an explicit Save, which makes commit opt-in by construction; a dismissal that also committed would leave Save with nothing to do and would silently rename the window on a stray click. What is discarded is a tag name and a colour swatch, never document content, and `openTagEditor` already re-seeded both fields from the stored tag on every open — so the discard needed no new code, only the exits. The tag editor has two entry points, and only the chip got a `stopPropagation` of its own. The other is `Home > Set window tag`, which runs `homeMenuOpen = false; openTagEditor();` with no `stopPropagation` — and `homeMenuOpen` being true means the window listener is ALREADY installed when it runs, so a click that reached the window would now open the popover and shut it again in one gesture. It does not reach the window: `.home-dropdown-menu` carries `onclick={(e) => e.stopPropagation()}`, which is why none of its dozen items need one each. That containment was untested and is now load-bearing for this fix, so a test drives the whole path — listener already installed, then the item's handler, then the container's — and asserts the editor is still open. Deleting the container's `stopPropagation` fails that test and only that test. The menu item is left as it is: giving one of twelve items its own `stopPropagation` would be redundant with the container and arbitrary next to its eleven siblings. The same question was asked of every other popover opener in the file. Only one other opens from inside another popover — the theme trigger, which is rendered into the kebab dropdown when the title bar is collapsed — and it calls `e.stopPropagation()` itself, so it is safe independently of its container. Every remaining opener (the Home, kebab and theme triggers, and the chip) stops propagation directly. Not changed: the three menu triggers do not close the tag editor when they open. They do not consistently close each other either (the theme trigger closes neither the Home nor the kebab menu), so completing that mutual exclusion is a change to the existing menus, not a dismiss path for this one. scripts/windowTagDismiss.test.ts runs the handlers rather than looking for them. A `.svelte` file cannot be imported by the Node test runner, so following homeTabRender.test.ts it lifts the component's real `openTagEditor` / `applyTag` / `clearTag` / `handleGlobalDismiss`, the two markup handlers on `.tag-editor`, whatever `onclick` the chip is actually wired to, the two handlers on the path from the Home-menu item, and the body of the `$effect`, into one scope over one set of component variables and the real `TabManager`, then dispatches events along a modelled bubble path. It asserts nothing about how any of it is spelled. Five tests cover the new exits. Four are fences that pass on the unfixed component: Enter still saves, ordinary typing does not close the popover, a click inside it does not dismiss it, and the Home-menu entry point still leaves it open. A popover that merely closed on everything would fail this file. Reverting only the component leaves those four green and turns the other five red on their own claims ("Escape left the popover open", "a second click on the chip left the popover open"), not on a missing attribute. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * feat(tags): show what a window tag covers, and hold each name once The chip is a coloured pill at the left end of the tab strip — exactly where Chrome puts a tab-GROUP chip, where a coloured enclosure marks which tabs belong to the group. Users read it the same way and ask which tabs are in this one. A window tag is not per-tab at all: `TabManager.windowTag` (`tabs.svelte.ts:183`) is a single nullable record, `Tab.svelte` and `TabList.svelte` never look at it, and every tab in the window is under the tag — including the one opened a second from now. The answer to "which tabs?" is "all of them", and nothing on screen says so. Four changes, none of which introduces per-tab membership. Two close defects; two are judgement calls about the mismatch above and are marked as such. DEFECT — the editor could not be dismissed without writing something. Fixed in the parent commit on this branch: Escape, a click elsewhere and a second click on the chip now close the popover, and only Save and Enter write a tag. DEFECT — two windows could hold one tag name, and the second one to close silently replaced the first's pinned document set. `save_pinned_tag_at` finds the entry in `pinned-tags.json` BY NAME and replaces its whole `files` list, so one name means one document set no matter how many windows carry it. #424's `pinned_tags` lock fixed the concurrent form of this — two interleaved read-modify-writes losing an update — and deliberately says nothing about two serialized writes each replacing a payload the other wrote, which is the ordinary outcome of two windows sharing a name and quitting. A regression test now pins that loss down before the rule that prevents it. `is_window_tag_taken` answers "does another live window carry this name?" out of `AppState.window_registry`, which already stores `tag_name` per window label and which the frontend keeps current through `set_window_meta`. No new registry: `WindowEvent::Destroyed` already removes the entry, so closing a window releases its name with no bookkeeping, and the registry is in memory, so a crash cannot leave a name blocked that nobody holds. The editor refuses a taken name in place — a line under the field, cleared as soon as the name is edited — rather than through a toast or a dialog, because the correction is made in the field the user is already in. Two decisions inside that rule: * Enforced ONLY at Save/Enter, never at session restore. A snapshot written by an older build can hold two windows under one name, and rejecting it at restore would silently clear a tag the user set, at a moment they are not looking at the popover. A restored duplicate costs a confusing pair of chips; clearing it costs state with nothing to recreate it from. An IPC failure lets the save through for the same reason: the check exists to stop one window overwriting another's pinned documents, and a broken command is not evidence that it would. * EVERY duplicate is blocked, not only pinned ones. This is deliberately broader than the mechanism: strictly, the overwrite only bites tags that are pinned, and two unpinned windows sharing a name harm no data. One rule is easier to explain and to remember than "duplicates are fine until you pin one", and two identical chips are confusing either way. JUDGEMENT — one scope line under the whole tab strip, in the tag's colour, rather than a per-tab underline. A per-tab underline makes belonging a property each tab carries, and there is no such property in the data: it would have to be invented, and then kept in sync with a window-level value that can change under it. A single strip-level line cannot get out of sync, and a newly opened tab is on it by construction rather than by anyone remembering to mark it. The bottom edge was free — `.tab.active` marks the active tab with `background-color` and nothing else in the strip uses that channel — so the two markings do not compete. The chip stays where it is and in the same colour, where it now reads as the line's legend. JUDGEMENT — right-click the chip opens the same popover left-click does, and the popover keeps all three of its controls. The visual borrowing above does NOT extend to Chrome's left/right division, and an earlier revision of this change that copied it was wrong. In Chrome, left-click collapses and expands the group, so the group's commands need a second surface to live on. A window tag has nothing to collapse — it scopes the whole window — so left-click has no second job, and moving Pin/Unpin and Remove to a right-click menu would only hide them behind a gesture that advertises nothing. The split was borrowed without the state that made it necessary. So Save, Pin/Unpin and Remove stay together in the popover, styled as #452 styled them, and right-click is a second way in. It differs from left-click in one respect: it opens but never closes, because `openTagEditor` re-seeds the draft from the stored tag and would discard a name being typed, and because a gesture whose purpose is "show me the tag's controls" should not sometimes hide them. It calls `preventDefault` so the platform menu does not open over the title bar, and `stopPropagation` for the reason the chip's left-click already does: `handleGlobalDismiss` is wired to window `contextmenu` and would otherwise dismiss the popover this just opened. NOT CHANGED — a new or detached window starts with no tag, and now has tests saying so. `restore()` returns early for any window that is not `main`, the transfer payload has no tag field, and `set_window_meta` only ever reports into the registry; nothing reads a registry `tag_name` back into a window. The tests drive all three paths, and a fence checks that the main window still does restore its own tag. Every behaviour above is tested by running it. `windowTagDismiss.test.ts`'s lifting of the component's real handlers moved into `windowTagEditor.ts` so that the scope line, the two entry points, the popover's three controls and the refusal drive the same running control over the same real `TabManager`, rather than a second copy free to drift. The two CSS assertions are the exception and say so in place: there is no layout engine in the runner, so they query the parsed stylesheet for the declarations that decide where the line is drawn — including that the active tab still marks itself with a background. Falsified per part, tests kept. Reverting the scope line alone turns the two scope-line tests red ("a tagged window drew no scope line", "no rule draws the scope line"). Reverting the right-click entry point and the popover's two commands turns four tests red ("the platform context menu was left to open over the title bar", "right-click closed the popover it exists to open", "the pin control does not offer to pin an unpinned tag", "a tagged window was not offered Pin"). Letting a non-main window restore turns the isolation test red ("a new window came up wearing the saved window tag"). Removing the exclusivity check turns three exclusivity tests red ("the duplicate name was written anyway"). Dropping the self-exclusion from the backend query turns `a_window_never_blocks_its_own_tag_name` red ("renaming the colour of a tag a window already holds must not be refused"). And making the pin file merge instead of replace turns the two-windows test red, which is what shows it executes the loss rather than describing it. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * fix(tags): unpin a tag whose name is cleared, as Remove Tag does There are two ways to take a tag off a window, and they disagreed about the saved session behind it. `clearTag` — Remove Tag — calls `remove_pinned_tag` first when the tag is pinned. `applyTag` with an emptied name called `tabManager.setWindowTag(null)` on its own, so the entry stayed in `pinned-tags.json`: the Home screen went on offering it as a reusable session under a name no window held any more, and reopening it handed the window back a tag the user had removed. Nothing announced this. The tag disappeared from the title bar, which is the whole visible result of the gesture, and the orphan only showed up the next time the user looked at Home. It is recoverable — unpin it there — but only by someone who works out that the two are the same thing. The empty-name branch now calls `clearTag()` instead of open-coding half of it, which is also what stops the two paths drifting apart again. `clearTag` already only asks the backend when `tag.pinned`, so an unpinned tag still costs no IPC. Two tests, both running the real `applyTag` against a recording `invoke`: one that a cleared pinned tag withdraws its session, one fence that a cleared unpinned tag withdraws nothing. Falsified by restoring the old two lines: the first goes red on "clearing the name orphaned the pinned session under a name no window holds", the fence stays green. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Fixes multi-window styling and hand-off issues, including native window drop shadow borders, overlay font styling, tag editor button design, and inter-window tab transfers.
Key Changes
window.set_shadow(true)is invoked for newly spawned detached/transfer windows (create_transfer_window) on Windows so DWM native drop shadows and borders are retained.font-family: var(--win-font)to.identify-flashinMarkdownViewer.svelteand set a global default font fallback onhtml, bodyinstyles.cssto prevent system serif font fallbacks during window identification highlights.TitleBar.svelte(.tag-save-btnand.tag-action-btn) for the window tag editor dialog (Save, Pin, and Clear controls).TabTransferBrokerin Rust to authorize explicit target window labels duringoffer_tab_to_window, resolving "unable to claim tab" errors when transferring tabs between existing windows.Verification
cargo test).npm test).