Skip to content

feat: remove hardcoded app names#2300

Draft
kantord wants to merge 24 commits into
mainfrom
issue-2296
Draft

feat: remove hardcoded app names#2300
kantord wants to merge 24 commits into
mainfrom
issue-2296

Conversation

@kantord
Copy link
Copy Markdown
Member

@kantord kantord commented May 29, 2026

see #2296

Notes:

  • APP_NAME needed to be changed, because it was used for various different things, such as referring to thv but also for package configuration. This means that I could not leave this const as it is. I change it's usages that were referring to the desktop app to use APP_DISPLAY_NAME (derived from package.json), then I basically renamed it to THV_DISPLAY_NAME to avoid confusion. This way the existing correct references to Toolhive (not Toolhive Studio) were kept unchanged, but with a different variable name
  • some usages of APP_NAME were referring to the desktop app, and explicitly diverging form the APP_DISPLAY_NAME format; in such ways that these usages cannot be derived from package.json without a deeper reworking and audit of the entire build process. It's not the right choice to do that here, so for those usages I was forced to create a "legacy const" named DEPRECATED_APP_NAME_FOR_BUILD so this issue will not spread.
  • In some cases a usage was slightly ambiguous or potentially confusing to the user, but also did not add any informational value. In some such cases I opted for simply removing the brand name, for instance: https://github.com/stacklok/toolhive-studio/pull/2300/changes#diff-78aca5d68c50f753ae7f2b0d719ff08d8a2bde69433349fa0080426147720644

Copilot AI review requested due to automatic review settings May 29, 2026 10:58
@kantord kantord marked this pull request as draft May 29, 2026 10:59
@kantord kantord changed the title chore: ban hardcoding brand names in eslint feat: remove hardcoded app names May 29, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds an experimental ESLint restriction intended to prevent hardcoding the ToolHive brand string in application TypeScript/TSX code, steering usage toward centralized constants in @common/app-info.

Changes:

  • Introduces a no-restricted-syntax rule to flag ToolHive occurrences in string literals, JSX text, and template literals across renderer/, main/, common/, and preload/.
  • Excludes tests and common/app-info.ts from the restriction.

Comment thread eslint.config.mjs
@kantord
Copy link
Copy Markdown
Member Author

kantord commented May 29, 2026

@claude i know this pr is not yet fully done, but can you please do a deep review of it for me?

make sure your summary is succint and you double validate every fiding

@claude
Copy link
Copy Markdown

claude Bot commented May 29, 2026

Claude finished @kantord's task in 5m 9s —— View job


Deep PR Review

This is a solid foundation — split into APP_DISPLAY_NAME (desktop product brand) vs THV_DISPLAY_NAME (engine), preload-derived dynamic brand in the renderer, and an ESLint rule to keep new code honest. Findings below were each verified by reading the referenced code.

Findings

1. APP_DISPLAY_NAME falls back to the literal 'ToolHive Studio' in the main process — rebranded builds will not actually rebrand main-process strings

common/app-info.ts:14-18 derives the name from globalThis.electronAPI?.appDisplayName. The preload bridge sets that in the renderer via preload/src/api/app.ts:10, but the main process has no preload — so globalThis.electronAPI is undefined and the fallback 'ToolHive Studio' is used unconditionally. Today no main-process file imports APP_DISPLAY_NAME, so it's not biting yet, but the moment you replace any main-side "ToolHive Studio" literal with the constant, rebranding breaks for that string. Consider deriving from app.getName() directly in main, or guarding the constant so it's renderer-only and main has a separate app.getName()-based getter. Fix this →

2. Quit dialog uses THV_DISPLAY_NAME for what is the desktop product

main/src/quit-confirmation.ts:59-61 renders Quit ${THV_DISPLAY_NAME} for a dialog where the user is quitting the desktop app. Per the comment you wrote at common/app-info.ts:22-24, THV_DISPLAY_NAME is the engine and is expected to stay "ToolHive" in rebranded builds — so a rebrand will leave a confusing "Quit ToolHive?" dialog on quit. Same semantic mismatch in renderer/src/common/components/starting-toolhive.tsx:59,68 ("Starting ToolHive configuration" — startup of the desktop app, not the engine) and renderer/src/routes/shutdown.tsx:29. AlreadyRunningError correctly uses THV_DISPLAY_NAME (it really is about the engine).

3. Regex literals match the new ESLint rule — main/src/utils/toolhive-version.ts:43 will fail lint

The selector Literal[value=/ToolHive/] matches all Literal nodes whose value (stringified) contains ToolHive. For a regex literal like /ToolHive-(.+?)-full\.nupkg/g, esquery coerces the RegExp via String() to /ToolHive-(...)/g, which matches. There's no eslint-disable on line 43, and the same issue applies to main/src/cli/cli-detection.ts:20 (/^ToolHive v(...)/m). Either disable per-line (the Squirrel artifact name and thv version output really are external strings tied to the engine), or refine the selector to exclude regex nodes — e.g. Literal[value=/ToolHive/][regex=undefined].

4. Still-unfixed user-visible literals that the rule will flag (non-exhaustive, all outside the test/mocks ignore list)

  • renderer/src/common/components/custom-socket-banner.tsx:42 — JSX text Using external ToolHive at
  • renderer/src/common/components/settings/registry/registry-tab.tsx:26 — JSX ToolHive default registry
  • renderer/src/common/components/settings/registry/registry-source-field.tsx:43 — JSX official ToolHive registry
  • renderer/src/common/components/settings/tabs/version-tab.tsx:131 — JSX ToolHive binary version
  • renderer/src/common/components/settings/tabs/components/external-cli-alert.tsx:20 — JSX An external ToolHive CLI was found at
  • renderer/src/features/agents/components/agent-detail-page.tsx:175 and agent-form-page.tsx:293 — JSX text
  • main/src/cli/path-configurator.ts:143, main/src/chat/agents/registry.ts:114 ('ToolHive Assistant'), and multiple strings in main/src/chat/agents/builtin-prompts.ts

You said in the PR thread you intend to replace these in this PR — flagging here as a track-the-list reference. Confirmed acknowledged via your earlier comment.

5. Mock appDisplayName is the engine name, not the product name

renderer/src/common/mocks/electronAPI.ts:9 returns 'ToolHive', but the real bridge returns app.getName() = productName = 'ToolHive Studio'. Tests that snapshot or assert on display strings will see a different value than production. Minor — likely benign since most tests don't read this — but worth aligning.

6. New eslint-disable no-restricted-syntax annotations on every log.* call leave a maintenance burden

The "temporarily ignore log related stuff" commit (839b72b) sprinkles // eslint-disable-next-line no-restricted-syntax -- TODO: decide on branding in logs across ~20 sites. These TODOs have no owner and will go stale silently. Two cleaner options: (a) refine the rule's selector to skip log-call arguments (e.g. CallExpression[callee.object.name='log'] Literal[value=/ToolHive/] excluded) so the disables aren't needed, or (b) replace with THV_DISPLAY_NAME now since log output identifying the engine reads naturally with that name.

7. Minor: globalThis typing in app-info.ts

The globalThis as unknown as { electronAPI?: ... } double-cast at common/app-info.ts:14-16 is unusual-enough to surprise a reader. If there is already a global type declaration for window.electronAPI (renderer ambient .d.ts), reusing it would avoid the double-assert. Cosmetic.

Things that look good

  • THV_DISPLAY_NAME / APP_DISPLAY_NAME split with documented semantics in common/app-info.ts is clear and correct in intent.
  • The preload sync-IPC pattern at app.ts:10 matches the existing mainLogPath pattern — consistent.
  • The spike test at preload/src/api/__tests__/app.test.ts directly proves the value passes through from main → preload → renderer.
  • The ESLint rule covers Literal, JSXText, and TemplateElement — all three branding-leak paths.
  • Sensible scope on the rule's ignores (tests, mocks, app-info.ts itself).

• Branch: issue-2296

@github-actions github-actions Bot added size/M and removed size/S labels May 29, 2026
Comment thread main/src/system-tray.ts
? `🟢 ${APP_NAME} is running`
: `🔴 ${APP_NAME} is stopped`,
? `🟢 ${app.getName()} is running`
: `🔴 ${app.getName()} is stopped`,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note: this is controversial: the logic is strictly about toolhive. So we might want to change this back to ToolHive. But it looks a bit off because nothign is explaining what ToolHive even is.

@kantord
Copy link
Copy Markdown
Member Author

kantord commented May 29, 2026

@claude another deep review please

@claude
Copy link
Copy Markdown

claude Bot commented May 29, 2026

Claude finished @kantord's task in 6m 25s —— View job


Deep PR Review (round 2)

Re-verified the prior findings against current code, then audited what's new. Picture has improved: quit-confirmation.ts now uses app.getName() directly (prior finding 2 partly resolved), and many string replacements have landed. But the underlying contract of APP_DISPLAY_NAME is broken in a way that several of the substitutions silently rely on.

Critical

1. APP_DISPLAY_NAME resolves to different values depending on where it's imported

  • package.json has "productName": "ToolHive" (not "ToolHive Studio").
  • app.getName() therefore returns "ToolHive", which is what the preload bridge sets at preload/src/api/app.ts:10.
  • In the rendererAPP_DISPLAY_NAME = "ToolHive".
  • In the main process / forge build (no preload bridge) → falls back to the literal 'ToolHive Studio' at common/app-info.ts:21.

The doc-comment immediately above says "derive from the live Electron app name (productName)" and the fallback claims to be that name — but the literal 'ToolHive Studio' does not equal the productName, so the two paths disagree. Concrete consequences I verified:

Pick one source of truth: either set productName: "ToolHive Studio" in package.json (and the fallback becomes consistent), or change the fallback to 'ToolHive' and accept APP_DISPLAY_NAME === THV_DISPLAY_NAME today. Without one of those, every new APP_DISPLAY_NAME import has a different meaning depending on which process loads it. Fix this →

Important

2. Lint will fail: regex literals trip the new rule

The selector Literal[value=/ToolHive/] matches any Literal whose stringified value contains ToolHive. esquery coerces a regex node's value via String(RegExp), which returns the source-with-delimiters — so it does contain ToolHive. Two sites have no eslint-disable:

Both legitimately match external strings tied to the engine — disable per-line, or refine the selector to exclude regex nodes with Literal[value=/ToolHive/][regex=undefined].

3. The 34 TODO: decide on branding in logs disables are a smell

I counted 34 TODO annotations vs 26 explanatory disables (e.g. "matches thv stderr output — must stay literal", "CLI install path segment"). The TODOs have no owner, no follow-up issue, and clutter every log.* call site. Two cleaner options:

  • (a) Refine the rule to skip log.*(...) call arguments — log lines aren't user-visible brand strings.
  • (b) Replace with THV_DISPLAY_NAME now — they all really do refer to the engine. The "decision" is already implicit in the constant split.

Either way, leaving 34 unowned TODOs on this PR pushes the cleanup work onto someone who doesn't have context.

Minor

4. system-tray.ts semantics (acknowledged controversial)

main/src/system-tray.ts:137-138 uses app.getName() for "🟢 ${name} is running" / "🔴 ${name} is stopped". As you noted on the review, the status menu item really is about the engine — THV_DISPLAY_NAME would be semantically correct, but a user with no engine knowledge will read the menu as "the app". Worth a one-line tooltip on the menu item ("MCP server status"), or accepting app.getName() because the productName already equals the engine name. The line 251 tooltip (tray.setToolTip(app.getName())) is correct as-is — it's the tray tooltip for the app itself.

5. starting-toolhive.tsx:60,70 uses THV_DISPLAY_NAME for desktop startup copy

"Starting ${THV_DISPLAY_NAME} configuration" and "checking your ${THV_DISPLAY_NAME} configuration" render during desktop-app boot, before any engine work happens. In a rebranded build (THV_DISPLAY_NAME stays "ToolHive" per design), the startup card would say "Starting ToolHive configuration" even though the user just launched, say, "Stacklok Desktop". Not load-bearing today, but inconsistent with the split's stated intent.

6. app.test.ts spike returns the same mock for both sync IPC calls

preload/src/api/__tests__/app.test.ts:5sendSync.mockReturnValue('Stacklok Desktop') is hit twice: once for get-main-log-path-sync (so mainLogPath is also 'Stacklok Desktop'), once for get-app-display-name. The test still passes because it only asserts on appDisplayName, but a reader sees mainLogPath = 'Stacklok Desktop' and pauses. mockImplementation((ch) => ch === 'get-app-display-name' ? 'Stacklok Desktop' : '/tmp/main.log') is clearer.

7. Previous review finding 5 was wrong — mock IS aligned

I had said the mock ('ToolHive') diverged from the real bridge ('ToolHive Studio'). Because the productName is actually "ToolHive", the real bridge also returns "ToolHive". The mock matches production today. Retract that point.

Things that look good

  • The quit-confirmation.ts switch to app.getName() (instead of THV_DISPLAY_NAME flagged in round 1) is the right move — rebrand-friendly and semantically a desktop-app action.
  • auto-launch.ts correctly uses app.getName() for the Linux .desktop entry — rebrand-friendly.
  • The "OSS thv project/API" explanatory disables in builtin-prompts.ts are well-justified — the agent instructions reference the literal product.
  • THV_DISPLAY_NAME usage in error/utils.ts METADATA_FIELDS label + ${THV_DISPLAY_NAME} Running: in the crash report is semantically correct (those are about the engine).
  • DEPRECATED_APP_NAME_FOR_BUILD is the right pragmatic compromise — the build configs need stable identifiers, and the DEPRECATED_ prefix is a clear flag.

Todo

  • Verify prior findings against current code
  • Audit APP_DISPLAY_NAME runtime values across processes
  • Confirm the ESLint regex-literal issue
  • Audit eslint-disable annotations (60 total: 34 TODO, 26 explanatory)
  • Cross-check tests/mocks
    • Branch: issue-2296

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants