From 01ba604aa622f80ffd3b6c83e70edfd6aee54b52 Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Fri, 10 Oct 2025 14:22:02 +0530 Subject: [PATCH 01/10] domain: De duplicate code in addDomain. --- app/renderer/js/utils/domain-util.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/app/renderer/js/utils/domain-util.ts b/app/renderer/js/utils/domain-util.ts index 0aed18954..28da71510 100644 --- a/app/renderer/js/utils/domain-util.ts +++ b/app/renderer/js/utils/domain-util.ts @@ -84,15 +84,13 @@ export async function addDomain(server: { if (server.icon) { const localIconUrl = await saveServerIcon(server.icon); server.icon = localIconUrl; - serverConfigSchema.parse(server); - database.push("/domains[]", server, true); - reloadDatabase(); } else { server.icon = defaultIconSentinel; - serverConfigSchema.parse(server); - database.push("/domains[]", server, true); - reloadDatabase(); } + + serverConfigSchema.parse(server); + database.push("/domains[]", server, true); + reloadDatabase(); } export function removeDomains(): void { From be75bafb84d88c3ea92800c95a48c41d2e6c4752 Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Fri, 10 Oct 2025 12:14:29 +0530 Subject: [PATCH 02/10] tab: Rename tabIndex to tabId. For `data-tab-id`, we were using tabIndex as a unique numeric incremental identifier. Having the word index in it was confusing since we have other index fields which do act like indexes. --- app/renderer/js/components/functional-tab.ts | 2 +- app/renderer/js/components/server-tab.ts | 2 +- app/renderer/js/components/tab.ts | 2 +- app/renderer/js/components/webview.ts | 6 ++--- app/renderer/js/main.ts | 28 ++++++++++---------- 5 files changed, 20 insertions(+), 20 deletions(-) diff --git a/app/renderer/js/components/functional-tab.ts b/app/renderer/js/components/functional-tab.ts index 72614deb2..7c1f5755e 100644 --- a/app/renderer/js/components/functional-tab.ts +++ b/app/renderer/js/components/functional-tab.ts @@ -43,7 +43,7 @@ export default class FunctionalTab extends Tab { templateHtml(): Html { return html` -
+
close
diff --git a/app/renderer/js/components/server-tab.ts b/app/renderer/js/components/server-tab.ts index b03c02e64..1d419fdf9 100644 --- a/app/renderer/js/components/server-tab.ts +++ b/app/renderer/js/components/server-tab.ts @@ -47,7 +47,7 @@ export default class ServerTab extends Tab { templateHtml(): Html { return html` -
+
diff --git a/app/renderer/js/components/tab.ts b/app/renderer/js/components/tab.ts index c4c949bc0..de502ff1d 100644 --- a/app/renderer/js/components/tab.ts +++ b/app/renderer/js/components/tab.ts @@ -8,7 +8,7 @@ export type TabProperties = { $root: Element; onClick: () => void; index: number; - tabIndex: number; + tabId: number; onHover?: () => void; onHoverOut?: () => void; materialIcon?: string; diff --git a/app/renderer/js/components/webview.ts b/app/renderer/js/components/webview.ts index 2e0bcf171..c450a1b3e 100644 --- a/app/renderer/js/components/webview.ts +++ b/app/renderer/js/components/webview.ts @@ -22,7 +22,7 @@ type WebViewProperties = { $root: Element; rootWebContents: WebContents; index: number; - tabIndex: number; + tabId: number; url: string; role: TabRole; isActive: () => boolean; @@ -48,7 +48,7 @@ export default class WebView { ×
; - tabIndex: number; + tabId: number; presetOrgs: string[]; preferenceView?: PreferenceView; constructor() { @@ -132,7 +132,7 @@ export class ServerManagerView { this.tabs = []; this.presetOrgs = []; this.functionalTabs = new Map(); - this.tabIndex = 0; + this.tabId = 0; } async init(): Promise { @@ -375,22 +375,22 @@ export class ServerManagerView { } initServer(server: ServerConfig, index: number): ServerTab { - const tabIndex = this.getTabIndex(); - const tab = new ServerTab({ + const tabId = this.gettabId(); + const tab: ServerTab = new ServerTab({ role: "server", icon: DomainUtil.iconAsUrl(server.icon), label: server.alias, $root: this.$tabsContainer, onClick: this.activateLastTab.bind(this, index), index, - tabIndex, + tabId, onHover: this.onHover.bind(this, index), onHoverOut: this.onHoverOut.bind(this, index), webview: WebView.create({ $root: this.$webviewsContainer, rootWebContents, index, - tabIndex, + tabId, url: server.url, role: "server", hasPermission: (origin: string, permission: string) => @@ -483,9 +483,9 @@ export class ServerManagerView { this.toggleDndButton(dnd); } - getTabIndex(): number { - const currentIndex = this.tabIndex; - this.tabIndex++; + gettabId(): number { + const currentIndex = this.tabId; + this.tabId++; return currentIndex; } @@ -574,7 +574,7 @@ export class ServerManagerView { const index = this.tabs.length; this.functionalTabs.set(tabProperties.page, index); - const tabIndex = this.getTabIndex(); + const tabId = this.gettabId(); const $view = await tabProperties.makeView(); this.$webviewsContainer.append($view); @@ -586,7 +586,7 @@ export class ServerManagerView { page: tabProperties.page, $root: this.$tabsContainer, index, - tabIndex, + tabId, onClick: this.activateTab.bind(this, index), onDestroy: async () => { await this.destroyFunctionalTab(tabProperties.page, index); @@ -811,8 +811,8 @@ export class ServerManagerView { } } - async isLoggedIn(tabIndex: number): Promise { - const tab = this.tabs[tabIndex]; + async isLoggedIn(tabId: number): Promise { + const tab = this.tabs[tabId]; if (!(tab instanceof ServerTab)) return false; const webview = await tab.webview; const url = webview.getWebContents().getURL(); @@ -1116,7 +1116,7 @@ export class ServerManagerView { (await tab.webview).webContentsId === webviewId ) { const concurrentTab: HTMLButtonElement = document.querySelector( - `div[data-tab-id="${CSS.escape(`${tab.properties.tabIndex}`)}"]`, + `div[data-tab-id="${CSS.escape(`${tab.properties.tabId}`)}"]`, )!; concurrentTab.click(); } From d0f0a661c95204757adb10c849fa64fd7a97017c Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Tue, 14 Oct 2025 16:07:58 +0530 Subject: [PATCH 03/10] domain: Add id field to server objects. This id will be passed around in events to uniquely identify a server. This id is not user generated, it is generated in app by domain-util.ts and stored in our JSON DB. This commit just adds the logic for generating and storing the id in the JSON DB. This commit does not make use of this newly added id, that will be done in further commits to keep the commits readable. In further commits, we will also start setting tabId in the rendered code to this id. We generate a random hex string for this id instead of having a numeric incremental ID. The reason for this is to avoid overhead of maintaining an ever increasingly numeric id while trying to make sure that ID collisions don't happen. Since we don't store our data in a real database, we can't have incremental primary keys by default and it makes little sense to implement that on our own, when this field is just being passed around to communicate b/w code and is not shown to humans. --- app/common/typed-ipc.ts | 4 +- app/common/types.ts | 3 ++ app/main/request.ts | 4 +- app/renderer/js/utils/domain-util.ts | 65 ++++++++++++++++++++++++---- 4 files changed, 64 insertions(+), 12 deletions(-) diff --git a/app/common/typed-ipc.ts b/app/common/typed-ipc.ts index ca10625a7..b010d4f40 100644 --- a/app/common/typed-ipc.ts +++ b/app/common/typed-ipc.ts @@ -1,5 +1,5 @@ import type {DndSettings} from "./dnd-util.ts"; -import type {MenuProperties, ServerConfig} from "./types.ts"; +import type {MenuProperties, ServerSettings} from "./types.ts"; export type MainMessage = { "clear-app-settings": () => void; @@ -26,7 +26,7 @@ export type MainMessage = { }; export type MainCall = { - "get-server-settings": (domain: string) => ServerConfig; + "get-server-settings": (domain: string) => ServerSettings; "is-online": (url: string) => boolean; "poll-clipboard": (key: Uint8Array, sig: Uint8Array) => string | undefined; "save-server-icon": (iconURL: string) => string | null; diff --git a/app/common/types.ts b/app/common/types.ts index def623e14..37b88cc77 100644 --- a/app/common/types.ts +++ b/app/common/types.ts @@ -12,6 +12,7 @@ export type NavigationItem = | "Shortcuts"; export type ServerConfig = { + id: string; url: string; alias: string; icon: string; @@ -19,6 +20,8 @@ export type ServerConfig = { zulipFeatureLevel: number; }; +export type ServerSettings = Omit; + export type TabRole = "server" | "function"; export type TabPage = "Settings" | "About"; diff --git a/app/main/request.ts b/app/main/request.ts index 4ffbc3dcd..aeb24f41e 100644 --- a/app/main/request.ts +++ b/app/main/request.ts @@ -10,7 +10,7 @@ import {z} from "zod"; import Logger from "../common/logger-util.ts"; import * as Messages from "../common/messages.ts"; -import type {ServerConfig} from "../common/types.ts"; +import type {ServerSettings} from "../common/types.ts"; /* Request: domain-util */ @@ -42,7 +42,7 @@ const generateFilePath = (url: string): string => { export const _getServerSettings = async ( domain: string, session: Session, -): Promise => { +): Promise => { const response = await session.fetch(domain + "/api/v1/server_settings"); if (!response.ok) { throw new Error(Messages.invalidZulipServerError(domain)); diff --git a/app/renderer/js/utils/domain-util.ts b/app/renderer/js/utils/domain-util.ts index 28da71510..cd5f1fd4d 100644 --- a/app/renderer/js/utils/domain-util.ts +++ b/app/renderer/js/utils/domain-util.ts @@ -1,3 +1,5 @@ +import assert from "node:assert"; +import {randomBytes} from "node:crypto"; import fs from "node:fs"; import path from "node:path"; @@ -11,7 +13,7 @@ import * as EnterpriseUtil from "../../../common/enterprise-util.ts"; import Logger from "../../../common/logger-util.ts"; import * as Messages from "../../../common/messages.ts"; import * as t from "../../../common/translation-util.ts"; -import type {ServerConfig} from "../../../common/types.ts"; +import type {ServerConfig, ServerSettings} from "../../../common/types.ts"; import defaultIcon from "../../img/icon.png"; import {ipcRenderer} from "../typed-ipc-renderer.ts"; @@ -19,11 +21,16 @@ const logger = new Logger({ file: "domain-util.log", }); +export function generateDomainId(): string { + return randomBytes(5).toString("hex"); +} + // For historical reasons, we store this string in domain.json to denote a // missing icon; it does not change with the actual icon location. export const defaultIconSentinel = "../renderer/img/icon.png"; -const serverConfigSchema = z.object({ +const storedServerSchema = z.object({ + id: z.string().optional(), url: z.url(), alias: z.string(), icon: z.string(), @@ -31,6 +38,18 @@ const serverConfigSchema = z.object({ zulipFeatureLevel: z.number().default(0), }); +const serverConfigSchema = storedServerSchema.extend({ + id: z.string(), +}); + +function addServerId(server: z.infer): ServerConfig { + assert.ok(server.id === undefined); + return serverConfigSchema.parse({ + ...server, + id: generateDomainId(), + }); +} + let database!: JsonDB; reloadDatabase(); @@ -88,8 +107,8 @@ export async function addDomain(server: { server.icon = defaultIconSentinel; } - serverConfigSchema.parse(server); - database.push("/domains[]", server, true); + const serverWithId = addServerId(storedServerSchema.parse(server)); + database.push("/domains[]", serverWithId, true); reloadDatabase(); } @@ -117,7 +136,7 @@ export function duplicateDomain(domain: string): boolean { export async function checkDomain( domain: string, silent = false, -): Promise { +): Promise { if (!silent && duplicateDomain(domain)) { // Do not check duplicate in silent mode throw new Error("This server has been added."); @@ -126,13 +145,13 @@ export async function checkDomain( domain = formatUrl(domain); try { - return await getServerSettings(domain); + return storedServerSchema.parse(await getServerSettings(domain)); } catch { throw new Error(Messages.invalidZulipServerError(domain)); } } -async function getServerSettings(domain: string): Promise { +async function getServerSettings(domain: string): Promise { return ipcRenderer.invoke("get-server-settings", domain); } @@ -151,7 +170,11 @@ export async function updateSavedServer( const serverConfig = getDomain(index); const oldIcon = serverConfig.icon; try { - const newServerConfig = await checkDomain(url, true); + const newServerSetting = await checkDomain(url, true); + const newServerConfig: ServerConfig = { + ...newServerSetting, + id: serverConfig.id, + }; const localIconUrl = await saveServerIcon(newServerConfig.icon); if (!oldIcon || localIconUrl !== defaultIconSentinel) { newServerConfig.icon = localIconUrl; @@ -168,6 +191,31 @@ export async function updateSavedServer( } } +function ensureDomainIds(): void { + try { + const domains = storedServerSchema + .array() + .parse(database.getObject("/domains")); + + let changed = false; + + const updatedDomains = domains.map((server) => { + if (server.id === undefined) { + changed = true; + server = addServerId(server); + } + + return server; + }); + + if (changed) { + database.push("/domains", updatedDomains, true); + } + } catch (error: unknown) { + if (!(error instanceof DataError)) throw error; + } +} + function reloadDatabase(): void { const domainJsonPath = path.join( app.getPath("userData"), @@ -192,6 +240,7 @@ function reloadDatabase(): void { } database = new JsonDB(domainJsonPath, true, true); + ensureDomainIds(); } export function formatUrl(domain: string): string { From bbdc51bce032fdb4645cb496685013b7116b7e5e Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Wed, 15 Oct 2025 12:48:49 +0530 Subject: [PATCH 04/10] ipc: Use id for `switch-server-tab` and set tabId to server id. We want to start using server id to communicate the identity of server in events. This commit does that for `switch-server-tab`. It also requires us to set tabId to server id instead of an incremental number. We try to make changes in this commit minimal and pass index to the functions that switch-server-tab calls. We will replace that index with tabId in further commits incrementally. --- app/common/typed-ipc.ts | 4 +-- app/common/types.ts | 2 ++ app/main/menu.ts | 24 ++++++++------- app/renderer/js/components/server-tab.ts | 7 +++-- app/renderer/js/components/tab.ts | 2 +- app/renderer/js/components/webview.ts | 2 +- app/renderer/js/main.ts | 30 +++++++++++-------- .../pages/preference/connected-org-section.ts | 1 + .../js/pages/preference/server-info-form.ts | 19 ++++++++++-- 9 files changed, 59 insertions(+), 32 deletions(-) diff --git a/app/common/typed-ipc.ts b/app/common/typed-ipc.ts index b010d4f40..638bc8106 100644 --- a/app/common/typed-ipc.ts +++ b/app/common/typed-ipc.ts @@ -14,7 +14,7 @@ export type MainMessage = { "realm-name-changed": (serverURL: string, realmName: string) => void; "reload-full-app": () => void; "save-last-tab": (index: number) => void; - "switch-server-tab": (index: number) => void; + "switch-server-tab": (serverId: string) => void; "toggle-app": () => void; "toggle-badge-option": (newValue: boolean) => void; "toggle-menubar": (showMenubar: boolean) => void; @@ -63,7 +63,7 @@ export type RendererMessage = { "set-idle": () => void; "show-keyboard-shortcuts": () => void; "show-notification-settings": () => void; - "switch-server-tab": (index: number) => void; + "switch-server-tab": (serverId: string) => void; "tab-devtools": () => void; "toggle-autohide-menubar": ( autoHideMenubar: boolean, diff --git a/app/common/types.ts b/app/common/types.ts index 37b88cc77..7d02b56ae 100644 --- a/app/common/types.ts +++ b/app/common/types.ts @@ -30,4 +30,6 @@ export type TabData = { page?: TabPage; label: string; index: number; + id: string; + serverId?: string; }; diff --git a/app/main/menu.ts b/app/main/menu.ts index ae1873895..08a91b662 100644 --- a/app/main/menu.ts +++ b/app/main/menu.ts @@ -329,7 +329,7 @@ function getWindowSubmenu( checked: tab.index === activeTabIndex, click(_item, focusedWindow) { if (focusedWindow) { - sendAction("switch-server-tab", tab.index); + sendAction("switch-server-tab", tab.serverId!); } }, type: "checkbox", @@ -348,7 +348,7 @@ function getWindowSubmenu( if (focusedWindow) { sendAction( "switch-server-tab", - getNextServer(tabs, activeTabIndex!), + getNextServer(tabs, tabs[activeTabIndex!]), ); } }, @@ -361,7 +361,7 @@ function getWindowSubmenu( if (focusedWindow) { sendAction( "switch-server-tab", - getPreviousServer(tabs, activeTabIndex!), + getPreviousServer(tabs, tabs[activeTabIndex!]), ); } }, @@ -704,20 +704,22 @@ async function checkForUpdate(): Promise { await appUpdater(true); } -function getNextServer(tabs: TabData[], activeTabIndex: number): number { +function getNextServer(tabs: TabData[], activeTab: TabData): string { + let {index} = activeTab; do { - activeTabIndex = (activeTabIndex + 1) % tabs.length; - } while (tabs[activeTabIndex]?.role !== "server"); + index = (index + 1) % tabs.length; + } while (tabs[index]?.role !== "server"); - return activeTabIndex; + return tabs[index].serverId!; } -function getPreviousServer(tabs: TabData[], activeTabIndex: number): number { +function getPreviousServer(tabs: TabData[], activeTab: TabData): string { + let {index} = activeTab; do { - activeTabIndex = (activeTabIndex - 1 + tabs.length) % tabs.length; - } while (tabs[activeTabIndex]?.role !== "server"); + index = (index - 1 + tabs.length) % tabs.length; + } while (tabs[index]?.role !== "server"); - return activeTabIndex; + return tabs[index].serverId!; } export function setMenu(properties: MenuProperties): void { diff --git a/app/renderer/js/components/server-tab.ts b/app/renderer/js/components/server-tab.ts index 1d419fdf9..ba6f886e6 100644 --- a/app/renderer/js/components/server-tab.ts +++ b/app/renderer/js/components/server-tab.ts @@ -9,19 +9,22 @@ import type WebView from "./webview.ts"; export type ServerTabProperties = { webview: Promise; + serverId: string; } & TabProperties; export default class ServerTab extends Tab { webview: Promise; + serverId: string; $el: Element; $name: Element; $icon: HTMLImageElement; $badge: Element; - constructor({webview, ...properties}: ServerTabProperties) { + constructor({webview, serverId, ...properties}: ServerTabProperties) { super(properties); this.webview = webview; + this.serverId = serverId; this.$el = generateNodeFromHtml(this.templateHtml()); this.properties.$root.append(this.$el); this.registerListeners(); @@ -84,7 +87,7 @@ export default class ServerTab extends Tab { const shownIndex = this.properties.index + 1; // Array index == Shown index - 1 - ipcRenderer.send("switch-server-tab", shownIndex - 1); + ipcRenderer.send("switch-server-tab", this.serverId); return process.platform === "darwin" ? `⌘${shownIndex}` diff --git a/app/renderer/js/components/tab.ts b/app/renderer/js/components/tab.ts index de502ff1d..a7e72a727 100644 --- a/app/renderer/js/components/tab.ts +++ b/app/renderer/js/components/tab.ts @@ -8,7 +8,7 @@ export type TabProperties = { $root: Element; onClick: () => void; index: number; - tabId: number; + tabId: string; onHover?: () => void; onHoverOut?: () => void; materialIcon?: string; diff --git a/app/renderer/js/components/webview.ts b/app/renderer/js/components/webview.ts index c450a1b3e..bc391a124 100644 --- a/app/renderer/js/components/webview.ts +++ b/app/renderer/js/components/webview.ts @@ -22,7 +22,7 @@ type WebViewProperties = { $root: Element; rootWebContents: WebContents; index: number; - tabId: number; + tabId: string; url: string; role: TabRole; isActive: () => boolean; diff --git a/app/renderer/js/main.ts b/app/renderer/js/main.ts index d0186659f..774cea753 100644 --- a/app/renderer/js/main.ts +++ b/app/renderer/js/main.ts @@ -86,7 +86,6 @@ export class ServerManagerView { activeTabIndex: number; tabs: ServerOrFunctionalTab[]; functionalTabs: Map; - tabId: number; presetOrgs: string[]; preferenceView?: PreferenceView; constructor() { @@ -132,7 +131,6 @@ export class ServerManagerView { this.tabs = []; this.presetOrgs = []; this.functionalTabs = new Map(); - this.tabId = 0; } async init(): Promise { @@ -375,7 +373,7 @@ export class ServerManagerView { } initServer(server: ServerConfig, index: number): ServerTab { - const tabId = this.gettabId(); + const tabId = this.generateTabId(); const tab: ServerTab = new ServerTab({ role: "server", icon: DomainUtil.iconAsUrl(server.icon), @@ -384,6 +382,7 @@ export class ServerManagerView { onClick: this.activateLastTab.bind(this, index), index, tabId, + serverId: server.id, onHover: this.onHover.bind(this, index), onHoverOut: this.onHoverOut.bind(this, index), webview: WebView.create({ @@ -483,10 +482,8 @@ export class ServerManagerView { this.toggleDndButton(dnd); } - gettabId(): number { - const currentIndex = this.tabId; - this.tabId++; - return currentIndex; + generateTabId(): string { + return DomainUtil.generateDomainId(); } async getCurrentActiveServer(): Promise { @@ -574,7 +571,7 @@ export class ServerManagerView { const index = this.tabs.length; this.functionalTabs.set(tabProperties.page, index); - const tabId = this.gettabId(); + const tabId = this.generateTabId(); const $view = await tabProperties.makeView(); this.$webviewsContainer.append($view); @@ -669,6 +666,7 @@ export class ServerManagerView { page: tab.properties.page, label: tab.properties.label, index: tab.properties.index, + id: tab.properties.tabId, })); } @@ -811,14 +809,21 @@ export class ServerManagerView { } } - async isLoggedIn(tabId: number): Promise { - const tab = this.tabs[tabId]; + async isLoggedIn(index: number): Promise { + const tab = this.tabs[index]; if (!(tab instanceof ServerTab)) return false; const webview = await tab.webview; const url = webview.getWebContents().getURL(); return !(url.endsWith("/login/") || webview.loading); } + getTabByServerId(serverId: string): ServerTab | undefined { + return this.tabs.find( + (tab): tab is ServerTab => + tab instanceof ServerTab && tab.serverId === serverId, + ); + } + addContextMenu($serverImg: HTMLElement, index: number): void { $serverImg.addEventListener("contextmenu", async (event) => { event.preventDefault(); @@ -1005,8 +1010,9 @@ export class ServerManagerView { ipcRenderer.send("reload-full-app"); }); - ipcRenderer.on("switch-server-tab", async (event, index: number) => { - await this.activateLastTab(index); + ipcRenderer.on("switch-server-tab", async (event, serverId: string) => { + const tab = this.getTabByServerId(serverId)!; + await this.activateLastTab(tab.properties.index); }); ipcRenderer.on("open-org-tab", async () => { diff --git a/app/renderer/js/pages/preference/connected-org-section.ts b/app/renderer/js/pages/preference/connected-org-section.ts index a6dd78038..6c8b15c4c 100644 --- a/app/renderer/js/pages/preference/connected-org-section.ts +++ b/app/renderer/js/pages/preference/connected-org-section.ts @@ -53,6 +53,7 @@ export function initConnectedOrgSection({ $root: $serverInfoContainer, server, index: i, + serverId: server.id, onChange: reloadApp, }); } diff --git a/app/renderer/js/pages/preference/server-info-form.ts b/app/renderer/js/pages/preference/server-info-form.ts index 8e440e18c..a05025200 100644 --- a/app/renderer/js/pages/preference/server-info-form.ts +++ b/app/renderer/js/pages/preference/server-info-form.ts @@ -12,6 +12,7 @@ type ServerInfoFormProperties = { $root: Element; server: ServerConfig; index: number; + serverId: string; onChange: () => void; }; @@ -70,14 +71,26 @@ export function initServerInfoForm(properties: ServerInfoFormProperties): void { }); $openServerButton.addEventListener("click", () => { - ipcRenderer.send("forward-message", "switch-server-tab", properties.index); + ipcRenderer.send( + "forward-message", + "switch-server-tab", + properties.serverId, + ); }); $serverInfoAlias.addEventListener("click", () => { - ipcRenderer.send("forward-message", "switch-server-tab", properties.index); + ipcRenderer.send( + "forward-message", + "switch-server-tab", + properties.serverId, + ); }); $serverIcon.addEventListener("click", () => { - ipcRenderer.send("forward-message", "switch-server-tab", properties.index); + ipcRenderer.send( + "forward-message", + "switch-server-tab", + properties.serverId, + ); }); } From 82483c0a0a49612eede583b86c70c06bc6a2b0c8 Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Tue, 14 Oct 2025 16:37:39 +0530 Subject: [PATCH 05/10] domain: Add getDomainById and updateDomainById. Use id instead of index to get and update domain. We remove updateDomain in this commit but not getDomain since it is being used in other places, where it will be easier to remove when changing the encompassing functions to use id instead of index. --- app/renderer/js/main.ts | 6 +++--- app/renderer/js/utils/domain-util.ts | 16 ++++++++++------ 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/app/renderer/js/main.ts b/app/renderer/js/main.ts index 774cea753..375b1b21d 100644 --- a/app/renderer/js/main.ts +++ b/app/renderer/js/main.ts @@ -332,7 +332,7 @@ export class ServerManagerView { (async () => { const serverConfig = await DomainUtil.updateSavedServer( server.url, - i, + server.id, ); tab.setLabel(serverConfig.alias); tab.setIcon(DomainUtil.iconAsUrl(serverConfig.icon)); @@ -1076,7 +1076,7 @@ export class ServerManagerView { const tab = this.tabs[index]; if (tab instanceof ServerTab) tab.setLabel(realmName); domain.alias = realmName; - DomainUtil.updateDomain(index, domain); + DomainUtil.updateDomainById(domain.id, domain); // Update the realm name also on the Window menu ipcRenderer.send("update-menu", { tabs: this.tabsForIpc, @@ -1098,7 +1098,7 @@ export class ServerManagerView { if (tab instanceof ServerTab) tab.setIcon(DomainUtil.iconAsUrl(localIconPath)); domain.icon = localIconPath; - DomainUtil.updateDomain(index, domain); + DomainUtil.updateDomainById(domain.id, domain); } }), ); diff --git a/app/renderer/js/utils/domain-util.ts b/app/renderer/js/utils/domain-util.ts index cd5f1fd4d..df6198152 100644 --- a/app/renderer/js/utils/domain-util.ts +++ b/app/renderer/js/utils/domain-util.ts @@ -89,9 +89,13 @@ export function getDomain(index: number): ServerConfig { ); } -export function updateDomain(index: number, server: ServerConfig): void { - reloadDatabase(); - serverConfigSchema.parse(server); +export function getDomainById(id: string): ServerConfig | undefined { + return getDomains().find((server) => server.id === id); +} + +export function updateDomainById(id: string, server: ServerConfig): void { + const index = getDomains().findIndex((domain) => domain.id === id); + assert.ok(index !== -1, `Domain with id ${id} not found`); database.push(`/domains[${index}]`, server, true); } @@ -164,10 +168,10 @@ export async function saveServerIcon(iconURL: string): Promise { export async function updateSavedServer( url: string, - index: number, + id: string, ): Promise { // Does not promise successful update - const serverConfig = getDomain(index); + const serverConfig = getDomainById(id)!; const oldIcon = serverConfig.icon; try { const newServerSetting = await checkDomain(url, true); @@ -178,7 +182,7 @@ export async function updateSavedServer( const localIconUrl = await saveServerIcon(newServerConfig.icon); if (!oldIcon || localIconUrl !== defaultIconSentinel) { newServerConfig.icon = localIconUrl; - updateDomain(index, newServerConfig); + updateDomainById(id, newServerConfig); reloadDatabase(); } From 465b006f1a4a93456f5d91cb4e8e638eb67bfb95 Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Fri, 17 Oct 2025 17:18:19 +0530 Subject: [PATCH 06/10] tab: Use activeTabId instead of activeTabIndex. We also use lastActiveTabId instead of lastActiveTabIndex now. `save-last-tab` uses id instead of index to commmunicate the identity of a server. getNextServer and getPreviousServer also start using id with this commit, the index mentioned in those functions is actually the order in which the servers should be displayed and we will fix that problem in upcoming commits. --- app/common/config-schemata.ts | 2 +- app/common/typed-ipc.ts | 2 +- app/common/types.ts | 2 +- app/main/index.ts | 12 ++-- app/main/menu.ts | 25 ++++---- app/renderer/js/main.ts | 115 +++++++++++++++++++--------------- 6 files changed, 87 insertions(+), 71 deletions(-) diff --git a/app/common/config-schemata.ts b/app/common/config-schemata.ts index 8d74f2583..87a7757a5 100644 --- a/app/common/config-schemata.ts +++ b/app/common/config-schemata.ts @@ -21,7 +21,7 @@ export const configSchemata = { downloadsPath: z.string(), enableSpellchecker: z.boolean(), errorReporting: z.boolean(), - lastActiveTab: z.number(), + lastActiveTabId: z.string().optional(), promptDownload: z.boolean(), proxyBypass: z.string(), // eslint-disable-next-line @typescript-eslint/naming-convention diff --git a/app/common/typed-ipc.ts b/app/common/typed-ipc.ts index 638bc8106..4874c34de 100644 --- a/app/common/typed-ipc.ts +++ b/app/common/typed-ipc.ts @@ -13,7 +13,7 @@ export type MainMessage = { "realm-icon-changed": (serverURL: string, iconURL: string) => void; "realm-name-changed": (serverURL: string, realmName: string) => void; "reload-full-app": () => void; - "save-last-tab": (index: number) => void; + "save-last-tab": (tabId: string) => void; "switch-server-tab": (serverId: string) => void; "toggle-app": () => void; "toggle-badge-option": (newValue: boolean) => void; diff --git a/app/common/types.ts b/app/common/types.ts index 7d02b56ae..6cdd148ab 100644 --- a/app/common/types.ts +++ b/app/common/types.ts @@ -1,6 +1,6 @@ export type MenuProperties = { tabs: TabData[]; - activeTabIndex?: number; + activeTabId?: string; enableMenu?: boolean; }; diff --git a/app/main/index.ts b/app/main/index.ts index d15de96cb..43643b532 100644 --- a/app/main/index.ts +++ b/app/main/index.ts @@ -428,9 +428,11 @@ function createMainWindow(): BrowserWindow { ipcMain.on("update-menu", (_event, properties: MenuProperties) => { AppMenu.setMenu(properties); - if (properties.activeTabIndex !== undefined) { - const activeTab = properties.tabs[properties.activeTabIndex]; - mainWindow.setTitle(`Zulip - ${activeTab.label}`); + if (properties.activeTabId !== undefined) { + const activeTab = properties.tabs.find( + (tab) => tab.id === properties.activeTabId, + ); + mainWindow.setTitle(`Zulip - ${activeTab!.label}`); } }); @@ -452,8 +454,8 @@ function createMainWindow(): BrowserWindow { }, ); - ipcMain.on("save-last-tab", (_event, index: number) => { - ConfigUtil.setConfigItem("lastActiveTab", index); + ipcMain.on("save-last-tab", (_event, tabId: string) => { + ConfigUtil.setConfigItem("lastActiveTabId", tabId); }); ipcMain.on("focus-this-webview", (event) => { diff --git a/app/main/menu.ts b/app/main/menu.ts index 08a91b662..51d4dcf58 100644 --- a/app/main/menu.ts +++ b/app/main/menu.ts @@ -294,7 +294,7 @@ function getHelpSubmenu(): MenuItemConstructorOptions[] { function getWindowSubmenu( tabs: TabData[], - activeTabIndex?: number, + activeTabId?: string, ): MenuItemConstructorOptions[] { const initialSubmenu: MenuItemConstructorOptions[] = [ { @@ -326,7 +326,7 @@ function getWindowSubmenu( label: tab.label, accelerator: tab.role === "function" ? "" : `${shortcutKey} + ${tab.index + 1}`, - checked: tab.index === activeTabIndex, + checked: tab.id === activeTabId, click(_item, focusedWindow) { if (focusedWindow) { sendAction("switch-server-tab", tab.serverId!); @@ -346,10 +346,7 @@ function getWindowSubmenu( enabled: tabs.length > 1, click(_item, focusedWindow) { if (focusedWindow) { - sendAction( - "switch-server-tab", - getNextServer(tabs, tabs[activeTabIndex!]), - ); + sendAction("switch-server-tab", getNextServer(tabs, activeTabId!)); } }, }, @@ -361,7 +358,7 @@ function getWindowSubmenu( if (focusedWindow) { sendAction( "switch-server-tab", - getPreviousServer(tabs, tabs[activeTabIndex!]), + getPreviousServer(tabs, activeTabId!), ); } }, @@ -375,7 +372,7 @@ function getWindowSubmenu( function getDarwinTpl( properties: MenuProperties, ): MenuItemConstructorOptions[] { - const {tabs, activeTabIndex, enableMenu = false} = properties; + const {tabs, activeTabId, enableMenu = false} = properties; return [ { @@ -525,7 +522,7 @@ function getDarwinTpl( }, { label: t.__("Window"), - submenu: getWindowSubmenu(tabs, activeTabIndex), + submenu: getWindowSubmenu(tabs, activeTabId), }, { label: t.__("Tools"), @@ -540,7 +537,7 @@ function getDarwinTpl( } function getOtherTpl(properties: MenuProperties): MenuItemConstructorOptions[] { - const {tabs, activeTabIndex, enableMenu = false} = properties; + const {tabs, activeTabId, enableMenu = false} = properties; return [ { label: t.__("File"), @@ -673,7 +670,7 @@ function getOtherTpl(properties: MenuProperties): MenuItemConstructorOptions[] { }, { label: t.__("Window"), - submenu: getWindowSubmenu(tabs, activeTabIndex), + submenu: getWindowSubmenu(tabs, activeTabId), }, { label: t.__("Tools"), @@ -704,7 +701,8 @@ async function checkForUpdate(): Promise { await appUpdater(true); } -function getNextServer(tabs: TabData[], activeTab: TabData): string { +function getNextServer(tabs: TabData[], activeTabId: string): string { + const activeTab = tabs.find((tab) => tab.id === activeTabId)!; let {index} = activeTab; do { index = (index + 1) % tabs.length; @@ -713,7 +711,8 @@ function getNextServer(tabs: TabData[], activeTab: TabData): string { return tabs[index].serverId!; } -function getPreviousServer(tabs: TabData[], activeTab: TabData): string { +function getPreviousServer(tabs: TabData[], activeTabId: string): string { + const activeTab = tabs.find((tab) => tab.id === activeTabId)!; let {index} = activeTab; do { index = (index - 1 + tabs.length) % tabs.length; diff --git a/app/renderer/js/main.ts b/app/renderer/js/main.ts index 375b1b21d..9aa2c14c1 100644 --- a/app/renderer/js/main.ts +++ b/app/renderer/js/main.ts @@ -83,9 +83,9 @@ export class ServerManagerView { $sidebar: Element; $fullscreenPopup: Element; loading: Set; - activeTabIndex: number; + activeTabId?: string; tabs: ServerOrFunctionalTab[]; - functionalTabs: Map; + functionalTabs: Map; presetOrgs: string[]; preferenceView?: PreferenceView; constructor() { @@ -127,7 +127,7 @@ export class ServerManagerView { ); this.loading = new Set(); - this.activeTabIndex = -1; + this.activeTabId = undefined; this.tabs = []; this.presetOrgs = []; this.functionalTabs = new Map(); @@ -195,7 +195,7 @@ export class ServerManagerView { // eslint-disable-next-line @typescript-eslint/naming-convention customCSS: false, silent: false, - lastActiveTab: 0, + lastActiveTabId: undefined, dnd: false, dndPreviousSettings: { showNotification: true, @@ -343,18 +343,28 @@ export class ServerManagerView { } // Open last active tab - let lastActiveTab = ConfigUtil.getConfigItem("lastActiveTab", 0); - if (lastActiveTab >= servers.length) { - lastActiveTab = 0; + const firstTab = this.tabs[0]; + let lastActiveTabId = ConfigUtil.getConfigItem( + "lastActiveTabId", + firstTab.properties.tabId, + )!; + const lastActiveTab = this.getTabById(lastActiveTabId); + // Set lastActiveTab to firstTab if lastActiveTab is undefined. + // It will be undefined if user disconnected the server for lastActiveTab. + if ( + lastActiveTab === undefined || + lastActiveTab.properties.index >= servers.length + ) { + lastActiveTabId = firstTab.properties.tabId; } - // `webview.load()` for lastActiveTab before the others - await this.activateTab(lastActiveTab); + // `webview.load()` for lastActiveTabId before the others + await this.activateTab(lastActiveTabId); await Promise.all( servers.map(async (server, i) => { - // After the lastActiveTab is activated, we load the others in the background + // After the lastActiveTabId is activated, we load the others in the background // without activating them, to prevent flashing of server icons - if (i === lastActiveTab) { + if (server.id === lastActiveTabId) { return; } @@ -379,7 +389,7 @@ export class ServerManagerView { icon: DomainUtil.iconAsUrl(server.icon), label: server.alias, $root: this.$tabsContainer, - onClick: this.activateLastTab.bind(this, index), + onClick: this.activateLastTab.bind(this, tabId), index, tabId, serverId: server.id, @@ -396,7 +406,7 @@ export class ServerManagerView { origin === server.url && permission === "notifications" && ConfigUtil.getConfigItem("showNotification", true), - isActive: () => index === this.activeTabIndex, + isActive: (): boolean => tabId === this.activeTabId, switchLoading: async (loading: boolean, url: string) => { if (loading) { this.loading.add(url); @@ -404,7 +414,7 @@ export class ServerManagerView { this.loading.delete(url); } - const tab = this.tabs[this.activeTabIndex]; + const tab = this.getTabById(this.activeTabId!); this.showLoading( tab instanceof ServerTab && this.loading.has((await tab.webview).properties.url), @@ -455,7 +465,7 @@ export class ServerManagerView { ); }); this.$reloadButton.addEventListener("click", async () => { - const tab = this.tabs[this.activeTabIndex]; + const tab = this.getTabById(this.activeTabId!); if (tab instanceof ServerTab) (await tab.webview).reload(); }); this.$addServerButton.addEventListener("click", async () => { @@ -465,7 +475,7 @@ export class ServerManagerView { await this.openSettings("General"); }); this.$backButton.addEventListener("click", async () => { - const tab = this.tabs[this.activeTabIndex]; + const tab = this.getTabById(this.activeTabId!); if (tab instanceof ServerTab) (await tab.webview).back(); }); @@ -487,7 +497,7 @@ export class ServerManagerView { } async getCurrentActiveServer(): Promise { - const tab = this.tabs[this.activeTabIndex]; + const tab = this.getTabById(this.activeTabId!); return tab instanceof ServerTab ? (await tab.webview).properties.url : ""; } @@ -569,9 +579,8 @@ export class ServerManagerView { } const index = this.tabs.length; - this.functionalTabs.set(tabProperties.page, index); - const tabId = this.generateTabId(); + this.functionalTabs.set(tabProperties.page, tabId); const $view = await tabProperties.makeView(); this.$webviewsContainer.append($view); @@ -584,9 +593,9 @@ export class ServerManagerView { $root: this.$tabsContainer, index, tabId, - onClick: this.activateTab.bind(this, index), + onClick: this.activateTab.bind(this, tabId), onDestroy: async () => { - await this.destroyFunctionalTab(tabProperties.page, index); + await this.destroyFunctionalTab(tabProperties.page, tabId); tabProperties.destroyView(); }, $view, @@ -597,7 +606,7 @@ export class ServerManagerView { // closed when the functional tab DOM is ready, handled in webview.js this.$webviewsContainer.classList.remove("loaded"); - await this.activateTab(this.functionalTabs.get(tabProperties.page)!); + await this.activateTab(tabId); } async openSettings( @@ -649,11 +658,11 @@ export class ServerManagerView { .loadURL(new URL("app/renderer/network.html", bundleUrl).href); } - async activateLastTab(index: number): Promise { - // Open all the tabs in background, also activate the tab based on the index - await this.activateTab(index); + async activateLastTab(tabId: string): Promise { + // Open all the tabs in background, also activate the tab based on id. + await this.activateTab(tabId); // Save last active tab via main process to avoid JSON DB errors - ipcRenderer.send("save-last-tab", index); + ipcRenderer.send("save-last-tab", tabId); } // Returns this.tabs in an way that does @@ -667,30 +676,33 @@ export class ServerManagerView { label: tab.properties.label, index: tab.properties.index, id: tab.properties.tabId, + serverId: tab instanceof ServerTab ? tab.serverId : undefined, })); } - async activateTab(index: number, hideOldTab = true): Promise { - const tab = this.tabs[index]; + async activateTab(id: string, hideOldTab = true): Promise { + const tab = this.getTabById(id); if (!tab) { return; } - if (this.activeTabIndex !== -1) { - if (this.activeTabIndex === index) { + if (this.activeTabId) { + if (this.activeTabId === tab.properties.tabId) { return; } + const previousTab = this.getTabById(this.activeTabId)!; + if (hideOldTab) { // If old tab is functional tab Settings, remove focus from the settings icon at sidebar bottom if ( - this.tabs[this.activeTabIndex].properties.role === "function" && - this.tabs[this.activeTabIndex].properties.page === "Settings" + previousTab.properties.role === "function" && + previousTab.properties.page === "Settings" ) { this.$settingsButton.classList.remove("active"); } - await this.tabs[this.activeTabIndex].deactivate(); + await previousTab.deactivate(); } } @@ -704,7 +716,7 @@ export class ServerManagerView { .classList.add("disable"); } - this.activeTabIndex = index; + this.activeTabId = tab.properties.tabId; await tab.activate(); this.showLoading( @@ -716,7 +728,7 @@ export class ServerManagerView { // JSON stringify this.tabs to avoid a crash // util.inspect is being used to handle circular references tabs: this.tabsForIpc, - activeTabIndex: this.activeTabIndex, + activeTabId: this.activeTabId, // Following flag controls whether a menu item should be enabled or not enableMenu: tab.properties.role === "server", }); @@ -727,20 +739,20 @@ export class ServerManagerView { this.$loadingIndicator.classList.toggle("hidden", !loading); } - async destroyFunctionalTab(page: TabPage, index: number): Promise { - const tab = this.tabs[index]; + async destroyFunctionalTab(page: TabPage, tabId: string): Promise { + const tab = this.getTabById(tabId)!; + if (tab instanceof ServerTab && (await tab.webview).loading) { return; } + delete this.tabs[tab.properties.index]; // eslint-disable-line @typescript-eslint/no-array-delete await tab.destroy(); - - delete this.tabs[index]; // eslint-disable-line @typescript-eslint/no-array-delete this.functionalTabs.delete(page); // Issue #188: If the functional tab was not focused, do not activate another tab. - if (this.activeTabIndex === index) { - await this.activateTab(0, false); + if (this.activeTabId === tabId) { + await this.activateTab(this.tabs[0].properties.tabId, false); } } @@ -749,7 +761,7 @@ export class ServerManagerView { this.$webviewsContainer.classList.remove("loaded"); // Clear global variables - this.activeTabIndex = -1; + this.activeTabId = undefined; this.tabs = []; this.functionalTabs.clear(); @@ -759,9 +771,8 @@ export class ServerManagerView { } async reloadView(): Promise { - // Save and remember the index of last active tab so that we can use it later - const lastActiveTab = this.tabs[this.activeTabIndex].properties.index; - ConfigUtil.setConfigItem("lastActiveTab", lastActiveTab); + // Save and remember the id of last active tab so that we can use it later + ConfigUtil.setConfigItem("lastActiveTabId", this.activeTabId); // Destroy the current view and re-initiate it this.destroyView(); @@ -824,6 +835,10 @@ export class ServerManagerView { ); } + getTabById(tabId: string): ServerOrFunctionalTab | undefined { + return this.tabs.find((tab) => tab.properties.tabId === tabId); + } + addContextMenu($serverImg: HTMLElement, index: number): void { $serverImg.addEventListener("contextmenu", async (event) => { event.preventDefault(); @@ -856,8 +871,8 @@ export class ServerManagerView { enabled: await this.isLoggedIn(index), click: async () => { // Switch to tab whose icon was right-clicked - await this.activateTab(index); const tab = this.tabs[index]; + await this.activateTab(tab.properties.tabId); if (tab instanceof ServerTab) (await tab.webview).showNotificationSettings(); }, @@ -942,7 +957,7 @@ export class ServerManagerView { for (const [channel, listener] of webviewListeners) { ipcRenderer.on(channel, async () => { - const tab = this.tabs[this.activeTabIndex]; + const tab = this.getTabById(this.activeTabId!); if (tab instanceof ServerTab) { const activeWebview = await tab.webview; if (activeWebview) listener(activeWebview); @@ -1012,7 +1027,7 @@ export class ServerManagerView { ipcRenderer.on("switch-server-tab", async (event, serverId: string) => { const tab = this.getTabByServerId(serverId)!; - await this.activateLastTab(tab.properties.index); + await this.activateLastTab(tab.properties.tabId); }); ipcRenderer.on("open-org-tab", async () => { @@ -1050,7 +1065,7 @@ export class ServerManagerView { if (updateMenu) { ipcRenderer.send("update-menu", { tabs: this.tabsForIpc, - activeTabIndex: this.activeTabIndex, + activeTabId: this.activeTabId, }); } }, @@ -1080,7 +1095,7 @@ export class ServerManagerView { // Update the realm name also on the Window menu ipcRenderer.send("update-menu", { tabs: this.tabsForIpc, - activeTabIndex: this.activeTabIndex, + activeTabId: this.activeTabId, }); } } From 7e97ff4c0343ee231cb5fb262088c35625ad3bb6 Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Wed, 15 Oct 2025 15:38:15 +0530 Subject: [PATCH 07/10] renderer: addContextMenu should not require index as an argument. We also replace pending getDomain() removals in favour of getDomainById() in this commit. We add a new function called removeDomainById as well. --- app/renderer/js/main.ts | 41 ++++++++++--------- .../pages/preference/connected-org-section.ts | 3 +- .../js/pages/preference/server-info-form.ts | 12 +++--- app/renderer/js/utils/domain-util.ts | 16 ++++---- 4 files changed, 36 insertions(+), 36 deletions(-) diff --git a/app/renderer/js/main.ts b/app/renderer/js/main.ts index 9aa2c14c1..0d02b9e67 100644 --- a/app/renderer/js/main.ts +++ b/app/renderer/js/main.ts @@ -442,14 +442,14 @@ export class ServerManagerView { initServerActions(): void { const $serverImgs: NodeListOf = document.querySelectorAll(".server-icons"); - for (const [index, $serverImg] of $serverImgs.entries()) { - this.addContextMenu($serverImg, index); + for (const $serverImg of $serverImgs) { + this.addContextMenu($serverImg); if ($serverImg.src === defaultIcon) { - this.displayInitialCharLogo($serverImg, index); + this.displayInitialCharLogo($serverImg); } $serverImg.addEventListener("error", () => { - this.displayInitialCharLogo($serverImg, index); + this.displayInitialCharLogo($serverImg); }); } } @@ -501,11 +501,7 @@ export class ServerManagerView { return tab instanceof ServerTab ? (await tab.webview).properties.url : ""; } - displayInitialCharLogo($img: HTMLImageElement, index: number): void { - // The index parameter is needed because webview[data-tab-id] can - // increment beyond the size of the sidebar org array and throw an - // error - + displayInitialCharLogo($img: HTMLImageElement): void { const $altIcon = document.createElement("div"); const $parent = $img.parentElement!; const $container = $parent.parentElement!; @@ -526,7 +522,7 @@ export class ServerManagerView { $img.remove(); $parent.append($altIcon); - this.addContextMenu($altIcon, index); + this.addContextMenu($altIcon); } sidebarHoverEvent( @@ -820,8 +816,8 @@ export class ServerManagerView { } } - async isLoggedIn(index: number): Promise { - const tab = this.tabs[index]; + async isLoggedIn(tabId: string): Promise { + const tab = this.getTabById(tabId); if (!(tab instanceof ServerTab)) return false; const webview = await tab.webview; const url = webview.getWebContents().getURL(); @@ -839,9 +835,16 @@ export class ServerManagerView { return this.tabs.find((tab) => tab.properties.tabId === tabId); } - addContextMenu($serverImg: HTMLElement, index: number): void { + addContextMenu($serverImg: HTMLElement): void { $serverImg.addEventListener("contextmenu", async (event) => { event.preventDefault(); + const tabElement = $serverImg.closest(".tab"); + const tabId = + tabElement instanceof HTMLElement + ? tabElement.dataset.tabId + : undefined; + if (tabId === undefined) return; + const template = [ { label: t.__("Disconnect organization"), @@ -855,11 +858,11 @@ export class ServerManagerView { ), }); if (response === 0) { - if (DomainUtil.removeDomain(index)) { + if (DomainUtil.removeDomainById(tabId)) { ipcRenderer.send("reload-full-app"); } else { const {title, content} = Messages.orgRemovalError( - DomainUtil.getDomain(index).url, + DomainUtil.getDomainById(tabId)!.url, ); dialog.showErrorBox(title, content); } @@ -868,11 +871,11 @@ export class ServerManagerView { }, { label: t.__("Notification settings"), - enabled: await this.isLoggedIn(index), + enabled: await this.isLoggedIn(tabId), click: async () => { // Switch to tab whose icon was right-clicked - const tab = this.tabs[index]; - await this.activateTab(tab.properties.tabId); + const tab = this.getTabById(tabId); + await this.activateTab(tabId); if (tab instanceof ServerTab) (await tab.webview).showNotificationSettings(); }, @@ -880,7 +883,7 @@ export class ServerManagerView { { label: t.__("Copy Zulip URL"), click() { - clipboard.writeText(DomainUtil.getDomain(index).url); + clipboard.writeText(DomainUtil.getDomainById(tabId)!.url); }, }, ]; diff --git a/app/renderer/js/pages/preference/connected-org-section.ts b/app/renderer/js/pages/preference/connected-org-section.ts index 6c8b15c4c..6c586b4a9 100644 --- a/app/renderer/js/pages/preference/connected-org-section.ts +++ b/app/renderer/js/pages/preference/connected-org-section.ts @@ -48,11 +48,10 @@ export function initConnectedOrgSection({ // Show noServerText if no servers are there otherwise hide it $existingServers.textContent = servers.length === 0 ? noServerText : ""; - for (const [i, server] of servers.entries()) { + for (const server of servers) { initServerInfoForm({ $root: $serverInfoContainer, server, - index: i, serverId: server.id, onChange: reloadApp, }); diff --git a/app/renderer/js/pages/preference/server-info-form.ts b/app/renderer/js/pages/preference/server-info-form.ts index a05025200..950d7d1e4 100644 --- a/app/renderer/js/pages/preference/server-info-form.ts +++ b/app/renderer/js/pages/preference/server-info-form.ts @@ -11,7 +11,6 @@ import * as DomainUtil from "../../utils/domain-util.ts"; type ServerInfoFormProperties = { $root: Element; server: ServerConfig; - index: number; serverId: string; onChange: () => void; }; @@ -59,13 +58,14 @@ export function initServerInfoForm(properties: ServerInfoFormProperties): void { message: t.__("Are you sure you want to disconnect this organization?"), }); if (response === 0) { - if (DomainUtil.removeDomain(properties.index)) { + if (DomainUtil.removeDomainById(properties.serverId)) { ipcRenderer.send("reload-full-app"); } else { - const {title, content} = Messages.orgRemovalError( - DomainUtil.getDomain(properties.index).url, - ); - dialog.showErrorBox(title, content); + const domain = DomainUtil.getDomainById(properties.serverId); + if (domain) { + const {title, content} = Messages.orgRemovalError(domain.url); + dialog.showErrorBox(title, content); + } } } }); diff --git a/app/renderer/js/utils/domain-util.ts b/app/renderer/js/utils/domain-util.ts index df6198152..eb8831d97 100644 --- a/app/renderer/js/utils/domain-util.ts +++ b/app/renderer/js/utils/domain-util.ts @@ -82,13 +82,6 @@ export function getDomains(): ServerConfig[] { } } -export function getDomain(index: number): ServerConfig { - reloadDatabase(); - return serverConfigSchema.parse( - database.getObject(`/domains[${index}]`), - ); -} - export function getDomainById(id: string): ServerConfig | undefined { return getDomains().find((server) => server.id === id); } @@ -121,8 +114,13 @@ export function removeDomains(): void { reloadDatabase(); } -export function removeDomain(index: number): boolean { - if (EnterpriseUtil.isPresetOrg(getDomain(index).url)) { +export function removeDomainById(id: string): boolean { + const index = getDomains().findIndex((domain) => domain.id === id); + if (index === -1) { + return false; + } + + if (EnterpriseUtil.isPresetOrg(getDomainById(id)!.url)) { return false; } From 73fd8b49a5fca8acd12ccf19936821d5ba8d584f Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Wed, 15 Oct 2025 16:13:38 +0530 Subject: [PATCH 08/10] renderer: onHoverOut and onHover should use tabId. --- app/renderer/js/main.ts | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/app/renderer/js/main.ts b/app/renderer/js/main.ts index 0d02b9e67..2a4f3754d 100644 --- a/app/renderer/js/main.ts +++ b/app/renderer/js/main.ts @@ -77,7 +77,6 @@ export class ServerManagerView { $reloadTooltip: HTMLElement; $loadingTooltip: HTMLElement; $settingsTooltip: HTMLElement; - $serverIconTooltip: HTMLCollectionOf; $backTooltip: HTMLElement; $dndTooltip: HTMLElement; $sidebar: Element; @@ -106,15 +105,6 @@ export class ServerManagerView { this.$loadingTooltip = $actionsContainer.querySelector("#loading-tooltip")!; this.$settingsTooltip = $actionsContainer.querySelector("#setting-tooltip")!; - - // TODO: This should have been querySelector but the problem is that - // querySelector doesn't return elements not present in dom whereas somehow - // getElementsByClassName does. To fix this we need to call this after this.initTabs - // is called in this.init. - // eslint-disable-next-line unicorn/prefer-query-selector - this.$serverIconTooltip = document.getElementsByClassName( - "server-tooltip", - ) as HTMLCollectionOf; this.$backTooltip = $actionsContainer.querySelector("#back-tooltip")!; this.$dndTooltip = $actionsContainer.querySelector("#dnd-tooltip")!; @@ -393,8 +383,8 @@ export class ServerManagerView { index, tabId, serverId: server.id, - onHover: this.onHover.bind(this, index), - onHoverOut: this.onHoverOut.bind(this, index), + onHover: this.onHover.bind(this, tabId), + onHoverOut: this.onHoverOut.bind(this, tabId), webview: WebView.create({ $root: this.$webviewsContainer, rootWebContents, @@ -546,20 +536,18 @@ export class ServerManagerView { }); } - onHover(index: number): void { - // `this.$serverIconTooltip[index].textContent` already has realm name, so we are just - // removing the style. - this.$serverIconTooltip[index].removeAttribute("style"); + onHover(tabId: string): void { + const tooltip = this.getServerTooltip(tabId)!; + tooltip.removeAttribute("style"); // To handle position of servers' tooltip due to scrolling of list of organizations // This could not be handled using CSS, hence the top of the tooltip is made same // as that of its parent element. - const {top} = - this.$serverIconTooltip[index].parentElement!.getBoundingClientRect(); - this.$serverIconTooltip[index].style.top = `${top}px`; + const {top} = tooltip.parentElement!.getBoundingClientRect(); + tooltip.style.top = `${top}px`; } - onHoverOut(index: number): void { - this.$serverIconTooltip[index].style.display = "none"; + onHoverOut(tabId: string): void { + this.getServerTooltip(tabId)!.style.display = "none"; } async openFunctionalTab(tabProperties: { @@ -835,6 +823,13 @@ export class ServerManagerView { return this.tabs.find((tab) => tab.properties.tabId === tabId); } + getServerTooltip(tabId: string): HTMLElement | undefined { + const tooltipElement = this.$tabsContainer.querySelector( + `.tab[data-tab-id="${CSS.escape(tabId)}"] .server-tooltip`, + ); + return tooltipElement instanceof HTMLElement ? tooltipElement : undefined; + } + addContextMenu($serverImg: HTMLElement): void { $serverImg.addEventListener("contextmenu", async (event) => { event.preventDefault(); From f28afbb421fc59acdee06490b5f6c402d8d331ed Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Thu, 16 Oct 2025 18:22:46 +0530 Subject: [PATCH 09/10] app: Don't access tabs by index. This commit removes miscallenous instances of accessing tabs directly by the index. --- app/main/menu.ts | 2 -- app/renderer/js/components/webview.ts | 5 ++--- app/renderer/js/main.ts | 21 +++++++++++---------- 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/app/main/menu.ts b/app/main/menu.ts index 51d4dcf58..5ca8c81e4 100644 --- a/app/main/menu.ts +++ b/app/main/menu.ts @@ -313,8 +313,6 @@ function getWindowSubmenu( type: "separator", }); for (const tab of tabs) { - // Skip missing elements left by `delete this.tabs[index]` in - // ServerManagerView. if (tab === undefined) continue; // Do not add functional tab settings to list of windows in menu bar diff --git a/app/renderer/js/components/webview.ts b/app/renderer/js/components/webview.ts index bc391a124..f203da42b 100644 --- a/app/renderer/js/components/webview.ts +++ b/app/renderer/js/components/webview.ts @@ -21,13 +21,12 @@ const shouldSilentWebview = ConfigUtil.getConfigItem("silent", false); type WebViewProperties = { $root: Element; rootWebContents: WebContents; - index: number; tabId: string; url: string; role: TabRole; isActive: () => boolean; switchLoading: (loading: boolean, url: string) => void; - onNetworkError: (index: number) => void; + onNetworkError: (id: string) => void; preload?: string; onTitleChange: () => void; hasPermission?: (origin: string, permission: string) => boolean; @@ -278,7 +277,7 @@ export default class WebView { if (hasConnectivityError) { console.error("error", errorDescription); if (!this.properties.url.includes("network.html")) { - this.properties.onNetworkError(this.properties.index); + this.properties.onNetworkError(this.properties.tabId); } } }); diff --git a/app/renderer/js/main.ts b/app/renderer/js/main.ts index 2a4f3754d..7039ed863 100644 --- a/app/renderer/js/main.ts +++ b/app/renderer/js/main.ts @@ -388,7 +388,6 @@ export class ServerManagerView { webview: WebView.create({ $root: this.$webviewsContainer, rootWebContents, - index, tabId, url: server.url, role: "server", @@ -410,8 +409,8 @@ export class ServerManagerView { this.loading.has((await tab.webview).properties.url), ); }, - onNetworkError: async (index: number) => { - await this.openNetworkTroubleshooting(index); + onNetworkError: async (tabId: string) => { + await this.openNetworkTroubleshooting(tabId); }, onTitleChange: this.updateBadge.bind(this), preload: url.pathToFileURL(path.join(bundlePath, "preload.cjs")).href, @@ -631,8 +630,8 @@ export class ServerManagerView { }); } - async openNetworkTroubleshooting(index: number): Promise { - const tab = this.tabs[index]; + async openNetworkTroubleshooting(id: string): Promise { + const tab = this.getTabById(id); if (!(tab instanceof ServerTab)) return; const webview = await tab.webview; const reconnectUtil = new ReconnectUtil(webview); @@ -730,7 +729,9 @@ export class ServerManagerView { return; } - delete this.tabs[tab.properties.index]; // eslint-disable-line @typescript-eslint/no-array-delete + this.tabs = this.tabs.filter( + (tabObject) => tabObject.properties.tabId !== tabId, + ); await tab.destroy(); this.functionalTabs.delete(page); @@ -1084,9 +1085,9 @@ export class ServerManagerView { ipcRenderer.on( "update-realm-name", (event, serverURL: string, realmName: string) => { - for (const [index, domain] of DomainUtil.getDomains().entries()) { + for (const domain of DomainUtil.getDomains()) { if (domain.url === serverURL) { - const tab = this.tabs[index]; + const tab = this.getTabById(domain.id); if (tab instanceof ServerTab) tab.setLabel(realmName); domain.alias = realmName; DomainUtil.updateDomainById(domain.id, domain); @@ -1104,10 +1105,10 @@ export class ServerManagerView { "update-realm-icon", async (event, serverURL: string, iconURL: string) => { await Promise.all( - DomainUtil.getDomains().map(async (domain, index) => { + DomainUtil.getDomains().map(async (domain) => { if (domain.url === serverURL) { const localIconPath = await DomainUtil.saveServerIcon(iconURL); - const tab = this.tabs[index]; + const tab = this.getTabById(domain.id); if (tab instanceof ServerTab) tab.setIcon(DomainUtil.iconAsUrl(localIconPath)); domain.icon = localIconPath; From 8f63f9e7588356ef7ba5aae2918f884598ef994c Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Thu, 23 Oct 2025 16:55:55 +0530 Subject: [PATCH 10/10] tab: Rename index to order. After introducing id field to the server object, index was just an indicator of the ordering of tabs in the tab list. This commit changes the name to accurately represent the field's function. This commit should make it easier to change the order of tabs in the future. Since order is not the same as index anymore, we add an additional function called getTabByOrder to replace the leftover instances of us fetching a tab by it's index when it meant to fetch a tab by it's id. After this commit, we should not have any instances of accessing tabs directly by their index in the server list or the tab list. --- app/common/types.ts | 2 +- app/main/menu.ts | 22 ++++++++++------- app/renderer/js/components/server-tab.ts | 6 ++--- app/renderer/js/components/tab.ts | 2 +- app/renderer/js/main.ts | 30 ++++++++++++++++-------- 5 files changed, 38 insertions(+), 24 deletions(-) diff --git a/app/common/types.ts b/app/common/types.ts index 6cdd148ab..fa5295419 100644 --- a/app/common/types.ts +++ b/app/common/types.ts @@ -29,7 +29,7 @@ export type TabData = { role: TabRole; page?: TabPage; label: string; - index: number; + order: number; id: string; serverId?: string; }; diff --git a/app/main/menu.ts b/app/main/menu.ts index 5ca8c81e4..718d036b4 100644 --- a/app/main/menu.ts +++ b/app/main/menu.ts @@ -323,7 +323,7 @@ function getWindowSubmenu( initialSubmenu.push({ label: tab.label, accelerator: - tab.role === "function" ? "" : `${shortcutKey} + ${tab.index + 1}`, + tab.role === "function" ? "" : `${shortcutKey} + ${tab.order + 1}`, checked: tab.id === activeTabId, click(_item, focusedWindow) { if (focusedWindow) { @@ -699,24 +699,28 @@ async function checkForUpdate(): Promise { await appUpdater(true); } +function getTabByOrder(tabs: TabData[], order: number): TabData | undefined { + return tabs.find((tab) => tab.order === order); +} + function getNextServer(tabs: TabData[], activeTabId: string): string { const activeTab = tabs.find((tab) => tab.id === activeTabId)!; - let {index} = activeTab; + let {order} = activeTab; do { - index = (index + 1) % tabs.length; - } while (tabs[index]?.role !== "server"); + order = (order + 1) % tabs.length; + } while (getTabByOrder(tabs, order)?.role !== "server"); - return tabs[index].serverId!; + return getTabByOrder(tabs, order)!.serverId!; } function getPreviousServer(tabs: TabData[], activeTabId: string): string { const activeTab = tabs.find((tab) => tab.id === activeTabId)!; - let {index} = activeTab; + let {order} = activeTab; do { - index = (index - 1 + tabs.length) % tabs.length; - } while (tabs[index]?.role !== "server"); + order = (order - 1) % tabs.length; + } while (getTabByOrder(tabs, order)?.role !== "server"); - return tabs[index].serverId!; + return getTabByOrder(tabs, order)!.serverId!; } export function setMenu(properties: MenuProperties): void { diff --git a/app/renderer/js/components/server-tab.ts b/app/renderer/js/components/server-tab.ts index ba6f886e6..f4cb67c3e 100644 --- a/app/renderer/js/components/server-tab.ts +++ b/app/renderer/js/components/server-tab.ts @@ -80,13 +80,13 @@ export default class ServerTab extends Tab { generateShortcutText(): string { // Only provide shortcuts for server [0..9] - if (this.properties.index >= 9) { + if (this.properties.order >= 9) { return ""; } - const shownIndex = this.properties.index + 1; + const shownIndex = this.properties.order + 1; - // Array index == Shown index - 1 + // Array index == Shown order - 1 ipcRenderer.send("switch-server-tab", this.serverId); return process.platform === "darwin" diff --git a/app/renderer/js/components/tab.ts b/app/renderer/js/components/tab.ts index a7e72a727..f18825f9a 100644 --- a/app/renderer/js/components/tab.ts +++ b/app/renderer/js/components/tab.ts @@ -7,7 +7,7 @@ export type TabProperties = { label: string; $root: Element; onClick: () => void; - index: number; + order: number; tabId: string; onHover?: () => void; onHoverOut?: () => void; diff --git a/app/renderer/js/main.ts b/app/renderer/js/main.ts index 7039ed863..ba3617eef 100644 --- a/app/renderer/js/main.ts +++ b/app/renderer/js/main.ts @@ -333,7 +333,7 @@ export class ServerManagerView { } // Open last active tab - const firstTab = this.tabs[0]; + const firstTab = this.getTabByOrder(this.tabs, 0)!; let lastActiveTabId = ConfigUtil.getConfigItem( "lastActiveTabId", firstTab.properties.tabId, @@ -343,7 +343,7 @@ export class ServerManagerView { // It will be undefined if user disconnected the server for lastActiveTab. if ( lastActiveTab === undefined || - lastActiveTab.properties.index >= servers.length + lastActiveTab.properties.order >= servers.length ) { lastActiveTabId = firstTab.properties.tabId; } @@ -351,14 +351,14 @@ export class ServerManagerView { // `webview.load()` for lastActiveTabId before the others await this.activateTab(lastActiveTabId); await Promise.all( - servers.map(async (server, i) => { + servers.map(async (server) => { // After the lastActiveTabId is activated, we load the others in the background // without activating them, to prevent flashing of server icons if (server.id === lastActiveTabId) { return; } - const tab = this.tabs[i]; + const tab = this.getTabByServerId(server.id); if (tab instanceof ServerTab) (await tab.webview).load(); }), ); @@ -372,7 +372,7 @@ export class ServerManagerView { } } - initServer(server: ServerConfig, index: number): ServerTab { + initServer(server: ServerConfig, order: number): ServerTab { const tabId = this.generateTabId(); const tab: ServerTab = new ServerTab({ role: "server", @@ -380,7 +380,7 @@ export class ServerManagerView { label: server.alias, $root: this.$tabsContainer, onClick: this.activateLastTab.bind(this, tabId), - index, + order, tabId, serverId: server.id, onHover: this.onHover.bind(this, tabId), @@ -561,7 +561,7 @@ export class ServerManagerView { return; } - const index = this.tabs.length; + const order = this.tabs.length; const tabId = this.generateTabId(); this.functionalTabs.set(tabProperties.page, tabId); const $view = await tabProperties.makeView(); @@ -574,7 +574,7 @@ export class ServerManagerView { label: tabProperties.label, page: tabProperties.page, $root: this.$tabsContainer, - index, + order, tabId, onClick: this.activateTab.bind(this, tabId), onDestroy: async () => { @@ -657,12 +657,19 @@ export class ServerManagerView { role: tab.properties.role, page: tab.properties.page, label: tab.properties.label, - index: tab.properties.index, + order: tab.properties.order, id: tab.properties.tabId, serverId: tab instanceof ServerTab ? tab.serverId : undefined, })); } + getTabByOrder( + tabs: ServerOrFunctionalTab[], + order: number, + ): ServerOrFunctionalTab | undefined { + return tabs.find((tab) => tab.properties.order === order); + } + async activateTab(id: string, hideOldTab = true): Promise { const tab = this.getTabById(id); if (!tab) { @@ -737,7 +744,10 @@ export class ServerManagerView { // Issue #188: If the functional tab was not focused, do not activate another tab. if (this.activeTabId === tabId) { - await this.activateTab(this.tabs[0].properties.tabId, false); + await this.activateTab( + this.getTabByOrder(this.tabs, 0)!.properties.tabId, + false, + ); } }