Skip to content

Use tab id and order field where appropriate instead of tabIndex. #1462

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Use tab id and order field where appropriate instead of tabIndex. #1462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
01ba604
domain: De duplicate code in addDomain.
shubham-padia Oct 10, 2025
be75baf
tab: Rename tabIndex to tabId.
shubham-padia Oct 10, 2025
d0f0a66
domain: Add id field to server objects.
shubham-padia Oct 14, 2025
bbdc51b
ipc: Use id for `switch-server-tab` and set tabId to server id.
shubham-padia Oct 15, 2025
82483c0
domain: Add getDomainById and updateDomainById.
shubham-padia Oct 14, 2025
465b006
tab: Use activeTabId instead of activeTabIndex.
shubham-padia Oct 17, 2025
7e97ff4
renderer: addContextMenu should not require index as an argument.
shubham-padia Oct 15, 2025
73fd8b4
renderer: onHoverOut and onHover should use tabId.
shubham-padia Oct 15, 2025
f28afbb
app: Don't access tabs by index.
shubham-padia Oct 16, 2025
8f63f9e
tab: Rename index to order.
shubham-padia Oct 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/common/config-schemata.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 5 additions & 5 deletions app/common/typed-ipc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -13,8 +13,8 @@ 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;
"switch-server-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;
"toggle-menubar": (showMenubar: boolean) => void;
Expand All @@ -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;
Expand Down Expand Up @@ -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,
Expand Down
9 changes: 7 additions & 2 deletions app/common/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
export type MenuProperties = {
tabs: TabData[];
activeTabIndex?: number;
activeTabId?: string;
enableMenu?: boolean;
};

Expand All @@ -12,19 +12,24 @@ export type NavigationItem =
| "Shortcuts";

export type ServerConfig = {
id: string;
Comment on lines 14 to +15
Copy link
Member

@gnprice gnprice Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OTOH if the ID property here is just the tab ID of some tab, rather than an independent ID for the server, then this should get a name like tabId that makes that explicit.

Copy link
Member Author

@shubham-padia shubham-padia Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an independent ID for the server. We set tabId to that independent id in the app.

Now that I think about it, if this seems confusing, it might be good to have two fields on the tab object, tabId can be a unique ID that is independent of the server id. And we store server id in a separate field called serverId, which will be undefined in case of functional tabs. Functional tabs are settings, reload, etc.

Let me know what you think of the current approach vs the proposed one vs anything else you might have in mind based on the explanation above, thanks!

Copy link
Member

@gnprice gnprice Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Probably a good discussion to have in a chat thread — that'll make it more visible for Anders and/or Tim to chime in too.

Copy link
Member Author

@shubham-padia shubham-padia Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#desktop > Early feedback for using Tab object instead of indices @ 💬

Copy link
Member Author

@shubham-padia shubham-padia Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made this change, ready for another look.

url: string;
alias: string;
icon: string;
zulipVersion: string;
zulipFeatureLevel: number;
};

export type ServerSettings = Omit<ServerConfig, "id">;

export type TabRole = "server" | "function";
export type TabPage = "Settings" | "About";

export type TabData = {
role: TabRole;
page?: TabPage;
label: string;
index: number;
order: number;
id: string;
serverId?: string;
};
12 changes: 7 additions & 5 deletions app/main/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}`);
}
});

Expand All @@ -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) => {
Expand Down
49 changes: 26 additions & 23 deletions app/main/menu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ function getHelpSubmenu(): MenuItemConstructorOptions[] {

function getWindowSubmenu(
tabs: TabData[],
activeTabIndex?: number,
activeTabId?: string,
): MenuItemConstructorOptions[] {
const initialSubmenu: MenuItemConstructorOptions[] = [
{
Expand All @@ -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
Expand All @@ -325,11 +323,11 @@ function getWindowSubmenu(
initialSubmenu.push({
label: tab.label,
accelerator:
tab.role === "function" ? "" : `${shortcutKey} + ${tab.index + 1}`,
checked: tab.index === activeTabIndex,
tab.role === "function" ? "" : `${shortcutKey} + ${tab.order + 1}`,
checked: tab.id === activeTabId,
click(_item, focusedWindow) {
if (focusedWindow) {
sendAction("switch-server-tab", tab.index);
sendAction("switch-server-tab", tab.serverId!);
}
},
type: "checkbox",
Expand All @@ -346,10 +344,7 @@ function getWindowSubmenu(
enabled: tabs.length > 1,
click(_item, focusedWindow) {
if (focusedWindow) {
sendAction(
"switch-server-tab",
getNextServer(tabs, activeTabIndex!),
);
sendAction("switch-server-tab", getNextServer(tabs, activeTabId!));
}
},
},
Expand All @@ -361,7 +356,7 @@ function getWindowSubmenu(
if (focusedWindow) {
sendAction(
"switch-server-tab",
getPreviousServer(tabs, activeTabIndex!),
getPreviousServer(tabs, activeTabId!),
);
}
},
Expand All @@ -375,7 +370,7 @@ function getWindowSubmenu(
function getDarwinTpl(
properties: MenuProperties,
): MenuItemConstructorOptions[] {
const {tabs, activeTabIndex, enableMenu = false} = properties;
const {tabs, activeTabId, enableMenu = false} = properties;

return [
{
Expand Down Expand Up @@ -525,7 +520,7 @@ function getDarwinTpl(
},
{
label: t.__("Window"),
submenu: getWindowSubmenu(tabs, activeTabIndex),
submenu: getWindowSubmenu(tabs, activeTabId),
},
{
label: t.__("Tools"),
Expand All @@ -540,7 +535,7 @@ function getDarwinTpl(
}

function getOtherTpl(properties: MenuProperties): MenuItemConstructorOptions[] {
const {tabs, activeTabIndex, enableMenu = false} = properties;
const {tabs, activeTabId, enableMenu = false} = properties;
return [
{
label: t.__("File"),
Expand Down Expand Up @@ -673,7 +668,7 @@ function getOtherTpl(properties: MenuProperties): MenuItemConstructorOptions[] {
},
{
label: t.__("Window"),
submenu: getWindowSubmenu(tabs, activeTabIndex),
submenu: getWindowSubmenu(tabs, activeTabId),
},
{
label: t.__("Tools"),
Expand Down Expand Up @@ -704,20 +699,28 @@ async function checkForUpdate(): Promise<void> {
await appUpdater(true);
}

function getNextServer(tabs: TabData[], activeTabIndex: number): number {
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 {order} = activeTab;
do {
activeTabIndex = (activeTabIndex + 1) % tabs.length;
} while (tabs[activeTabIndex]?.role !== "server");
order = (order + 1) % tabs.length;
} while (getTabByOrder(tabs, order)?.role !== "server");

return activeTabIndex;
return getTabByOrder(tabs, order)!.serverId!;
}

function getPreviousServer(tabs: TabData[], activeTabIndex: number): number {
function getPreviousServer(tabs: TabData[], activeTabId: string): string {
const activeTab = tabs.find((tab) => tab.id === activeTabId)!;
let {order} = activeTab;
do {
activeTabIndex = (activeTabIndex - 1 + tabs.length) % tabs.length;
} while (tabs[activeTabIndex]?.role !== "server");
order = (order - 1) % tabs.length;
} while (getTabByOrder(tabs, order)?.role !== "server");

return activeTabIndex;
return getTabByOrder(tabs, order)!.serverId!;
}

export function setMenu(properties: MenuProperties): void {
Expand Down
4 changes: 2 additions & 2 deletions app/main/request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */

Expand Down Expand Up @@ -42,7 +42,7 @@ const generateFilePath = (url: string): string => {
export const _getServerSettings = async (
domain: string,
session: Session,
): Promise<ServerConfig> => {
): Promise<ServerSettings> => {
const response = await session.fetch(domain + "/api/v1/server_settings");
if (!response.ok) {
throw new Error(Messages.invalidZulipServerError(domain));
Expand Down
2 changes: 1 addition & 1 deletion app/renderer/js/components/functional-tab.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export default class FunctionalTab extends Tab {

templateHtml(): Html {
return html`
<div class="tab functional-tab" data-tab-id="${this.properties.tabIndex}">
<div class="tab functional-tab" data-tab-id="${this.properties.tabId}">
<div class="server-tab-badge close-button">
<i class="material-icons">close</i>
</div>
Expand Down
15 changes: 9 additions & 6 deletions app/renderer/js/components/server-tab.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,22 @@ import type WebView from "./webview.ts";

export type ServerTabProperties = {
webview: Promise<WebView>;
serverId: string;
} & TabProperties;

export default class ServerTab extends Tab {
webview: Promise<WebView>;
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();
Expand All @@ -47,7 +50,7 @@ export default class ServerTab extends Tab {

templateHtml(): Html {
return html`
<div class="tab" data-tab-id="${this.properties.tabIndex}">
<div class="tab" data-tab-id="${this.properties.tabId}">
<div class="server-tooltip" style="display:none">
${this.properties.label}
</div>
Expand Down Expand Up @@ -77,14 +80,14 @@ 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
ipcRenderer.send("switch-server-tab", shownIndex - 1);
// Array index == Shown order - 1
ipcRenderer.send("switch-server-tab", this.serverId);

return process.platform === "darwin"
? `⌘${shownIndex}`
Expand Down
4 changes: 2 additions & 2 deletions app/renderer/js/components/tab.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ export type TabProperties = {
label: string;
$root: Element;
onClick: () => void;
index: number;
tabIndex: number;
order: number;
tabId: string;
onHover?: () => void;
onHoverOut?: () => void;
materialIcon?: string;
Expand Down
11 changes: 5 additions & 6 deletions app/renderer/js/components/webview.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,12 @@ const shouldSilentWebview = ConfigUtil.getConfigItem("silent", false);
type WebViewProperties = {
$root: Element;
rootWebContents: WebContents;
index: number;
tabIndex: 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;
Expand All @@ -48,7 +47,7 @@ export default class WebView {
<span class="webview-unsupported-dismiss">×</span>
</div>
<webview
data-tab-id="${properties.tabIndex}"
data-tab-id="${properties.tabId}"
src="${properties.url}"
${properties.preload === undefined
? html``
Expand Down Expand Up @@ -89,7 +88,7 @@ export default class WebView {
}

const selector = `webview[data-tab-id="${CSS.escape(
`${properties.tabIndex}`,
`${properties.tabId}`,
)}"]`;
const webContentsId: unknown =
await properties.rootWebContents.executeJavaScript(
Expand Down Expand Up @@ -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);
}
}
});
Expand Down
Loading