refactor(release): simplify the favicon lookup budget, share the link-name cap, read sim/ queries as Next parses them

Author: waleedlatif1

apps/extension/DESIGN.md

@@ -64,9 +64,10 @@ other workspace resources or browser-control tools.
   or arbitrary remote code execution. The only network interception is the static
   `sim/` redirect rule above.
 - The only background network request is the `sim` address-bar keyword: after
-  `sim` and Space, the typed link name (never a page, URL, or search containing a
-  space) is sent to the configured Sim origin with the signed-in session to suggest
-  matching Sim links. Unlisted links are never suggested.
+  `sim` and Space, the text typed before the first `/` or `?` is sent to the
+  configured Sim origin with the signed-in session to suggest matching Sim links,
+  unless it contains a space (a search) or is longer than a link name can be. Page
+  content is never sent, and unlisted links are never suggested.
 - No always-running page reader. Extraction is tied to an explicit attachment action.
 - Page bytes are not written to extension local/sync storage. Temporary context-menu
   handoff uses trusted-context session storage, expiration and navigation cleanup.

apps/extension/README.md

@@ -27,7 +27,7 @@ SIM_EXTENSION_DEV=1 SIM_EXTENSION_ORIGIN=http://localhost:3000 bun run --cwd app
 
 - Required capabilities: `activeTab`, `scripting`, `sidePanel`, `storage`, `contextMenus`, `declarativeNetRequestWithHostAccess`, and access to the configured Sim origin and to `http(s)://sim/*`.
 - Typing `sim/<name>` in the address bar opens the organization's [Sim link](https://docs.sim.ai/search/sim-links). A single static redirect rule rewrites top-level navigations to `http(s)://sim/*` to `<Sim origin>/sim/*` with the path and query unchanged. The `sim` host permission exists only for that rule: every navigation to that host is redirected before it loads, and no other request is matched or observed. An organization that already serves an intranet host named `sim` should not install this build, since its links would be redirected to Sim.
-- The `sim` address-bar keyword (`sim`, then Space) lists matching Sim links while you type and opens what you enter through the same resolver. It is the extension's only network request: the background worker sends the typed link name, and nothing else (input containing a space is treated as a search and sends nothing), to `<Sim origin>/api/sim-links/suggestions` with the signed-in session, and the extension's content security policy allows connections to that origin alone. Unlisted links are never suggested.
+- The `sim` address-bar keyword (`sim`, then Space) lists matching Sim links while you type and opens what you enter through the same resolver. It is the extension's only network request: the background worker sends the text typed before the first `/` or `?`, and nothing else, unless that text contains a space (a search) or is longer than a link name can be, to `<Sim origin>/api/sim-links/suggestions` with the signed-in session, and the extension's content security policy allows connections to that origin alone. Unlisted links are never suggested.
 - Optional HTTP/HTTPS site access is requested only by a menu action naming that site. Chrome's `activeTab` access is temporary and does not follow arbitrary new sites.
 - Page capture runs on demand in Chrome's isolated world. It excludes editable fields, inputs, hidden elements, scripts, and embedded documents. It reads only the top frame. PDF viewers and canvas documents may have no accessible text.
 - Extraction stops at 50,000 text characters, 150,000 serialized UTF-8 bytes, 15,000 visited nodes, or 150ms. The composer exposes truncation.

apps/extension/src/omnibox.ts

@@ -1,4 +1,5 @@
 import {
+  SIM_LINK_NAME_MAX_LENGTH,
   SIM_LINK_SUGGESTIONS_PATH,
   simLinkSuggestionsSchema,
 } from '@sim/browser-extension-protocol'
@@ -12,9 +13,6 @@ const logger = createLogger('BrowserExtensionOmnibox')
 /** Longer than the gap between a typist's keystrokes, so a name is looked up once it settles. */
 const SUGGEST_DELAY_MS = 220
 
-/** Link names are at most this long, so nothing longer can match. */
-const MAX_NAME_LENGTH = 64
-
 /** A Sim link as suggested: its name, and what it shows beside the name. */
 interface SuggestedLink {
   name: string
@@ -62,15 +60,17 @@ function encodeSegment(segment: string): string {
 
 /**
  * The resolver address for what was typed, as the address bar would send `sim/<typed>`: each
- * `/` starts a value, kept in position even when empty, and a trailing `/` adds none.
+ * `/` starts a value, kept in position even when empty, and a trailing `/` adds none. The query
+ * is passed on as typed.
  */
 function simLinkUrl(text: string): string {
   const { name, rest } = readTyped(text)
   const queryStart = rest.indexOf('?')
   const path = `${name}${queryStart === -1 ? rest : rest.slice(0, queryStart)}`.replace(/\/$/, '')
-  const query = queryStart === -1 ? '' : new URLSearchParams(rest.slice(queryStart + 1)).toString()
   const segments = path ? path.split('/').map(encodeSegment) : []
-  return `${SIM_ORIGIN}/${['sim', ...segments].join('/')}${query ? `?${query}` : ''}`
+  const url = new URL(`${SIM_ORIGIN}/${['sim', ...segments].join('/')}`)
+  if (queryStart !== -1) url.search = rest.slice(queryStart + 1)
+  return url.toString()
 }
 
 /** The links Sim suggests for `name`, or `null` when it couldn't answer (signed out, rate limited). */
@@ -119,7 +119,7 @@ export function registerSimLinkOmnibox() {
       description: `Open <match>sim/${escapeXml(`${name}${rest}`)}</match>`,
     })
     // A name can't hold a space, so `sim card prices` is a search, not a link: ask Sim nothing.
-    if (/\s/.test(name) || name.length > MAX_NAME_LENGTH) {
+    if (/\s/.test(name) || name.length > SIM_LINK_NAME_MAX_LENGTH) {
       suggest([])
       return
     }

apps/sim/app/o/[organizationId]/layout.tsx

@@ -1,5 +1,4 @@
 import { createLogger } from '@sim/logger'
-import { getErrorMessage } from '@sim/utils/errors'
 import { dehydrate, HydrationBoundary } from '@tanstack/react-query'
 import { cookies } from 'next/headers'
 import { redirect } from 'next/navigation'
@@ -66,15 +65,7 @@ export default async function OrganizationLayout({
   if (!context) {
     return <OrganizationAccessDenied />
   }
-  const peopleEnabledLookup = canBrowsePeople(organizationId, context.viewer.role).catch(
-    (error) => {
-      logger.warn('Could not resolve People navigation access', {
-        organizationId,
-        error: getErrorMessage(error),
-      })
-      return false
-    }
-  )
+  const peopleEnabledLookup = canBrowsePeople(organizationId, context.viewer.role)
   const simLinksEnabledLookup = isSimLinksEnabled(organizationId)
   if (
     !context.mothershipAvailable &&

apps/sim/app/o/[organizationId]/links/sim-link-modal.tsx

@@ -1,6 +1,7 @@
 'use client'
 
 import { useId, useState } from 'react'
+import { SIM_LINK_NAME_MAX_LENGTH } from '@sim/browser-extension-protocol'
 import {
   Checkbox,
   ChipModal,
@@ -16,7 +17,6 @@ import type { SimLink, SimLinkBody } from '@/lib/api/contracts/sim-links'
 import {
   isVariableSimLinkUrl,
   SIM_LINK_DESCRIPTION_MAX_LENGTH,
-  SIM_LINK_NAME_MAX_LENGTH,
   SIM_LINK_VARIABLE,
   simLinkFieldsSchema,
   toSimLinkName,

apps/sim/app/sim/[[...path]]/page.tsx

@@ -1,12 +1,10 @@
 import { ChipLink, StatusPageContent } from '@sim/emcn'
 import type { Metadata } from 'next'
-import { headers } from 'next/headers'
 import { notFound, redirect } from 'next/navigation'
 import { getSession } from '@/lib/auth'
 import { organizationRoutes, simLinkPath } from '@/lib/navigation/paths'
 import { resolveSimLinkOrganization } from '@/lib/sim-links/application/organization'
 import { resolveSimLink, type SimLinkResolution } from '@/lib/sim-links/application/use-cases'
-import { SIM_LINK_QUERY_HEADER } from '@/lib/sim-links/typed-query'
 import { parseTypedSimLink, simLinkNameSchema } from '@/lib/sim-links/validation'
 import { buildAuthCrossLink } from '@/app/(auth)/auth-redirect'
 import { LogoShell } from '@/app/(landing)/components/logo-shell'
@@ -19,6 +17,7 @@ export const metadata: Metadata = {
 
 interface SimLinkPageProps {
   params: Promise<{ path?: string[] }>
+  searchParams: Promise<Record<string, string | string[] | undefined>>
 }
 
 /**
@@ -32,6 +31,18 @@ function decodeSimLinkPath(path: string[]): string[] {
   return segments.length === 1 ? segments[0].split('/').filter(Boolean) : segments
 }
 
+/**
+ * The query, from the params Next has already parsed and normalized (it re-serializes every
+ * request's query before app code sees it, so the typed spelling isn't recoverable here).
+ */
+function toQueryString(searchParams: Record<string, string | string[] | undefined>): string {
+  const query = new URLSearchParams()
+  for (const [key, value] of Object.entries(searchParams)) {
+    for (const item of [value ?? []].flat()) query.append(key, item)
+  }
+  return query.toString()
+}
+
 /**
  * Why a link did not redirect. Only a valid link name is echoed back, so an arbitrary path
  * can't put its own text on this page.
@@ -50,9 +61,8 @@ function describeUnresolved(result: SimLinkResolution, validName: boolean, name:
  * Where `sim/<name>/<values…>?<query>` lands: the viewer's organization link's destination,
  * or a page that offers to create the link when nobody has yet.
  */
-export default async function SimLinkPage({ params }: SimLinkPageProps) {
-  const [{ path = [] }, requestHeaders] = await Promise.all([params, headers()])
-  const query = requestHeaders.get(SIM_LINK_QUERY_HEADER) ?? ''
+export default async function SimLinkPage({ params, searchParams }: SimLinkPageProps) {
+  const [{ path = [] }, query] = await Promise.all([params, searchParams.then(toQueryString)])
   const segments = decodeSimLinkPath(path)
   const session = await getSession()
   if (!session?.user)

apps/sim/lib/api/server/routes/definition.ts

@@ -24,7 +24,7 @@ export function requireJsonRouteDefinition(
 }
 
 /** Statuses a response body can't accompany, so never a binary route's default. */
-const BODYLESS_STATUSES = new Set([204, 205, 304])
+const BODYLESS_STATUSES = new Set([204, 205])
 
 export function requireBinaryRouteDefinition(
   contract: AnyApiRouteContract,

apps/sim/lib/favicon/application.ts

@@ -2,13 +2,13 @@ import type { SessionPrincipal } from '@sim/auth/principal'
 import { defineOperation } from '@/lib/core/application'
 import { RateLimiter, type TokenBucketConfig } from '@/lib/core/rate-limiter'
 import { defineAuthorizedCredentialUserUseCase } from '@/lib/credentials/application/authorized-user-use-case'
-import { resolveFavicon } from '@/lib/favicon/resolve.server'
+import { cachedFavicon, type FaviconResolution, resolveFavicon } from '@/lib/favicon/resolve.server'
 
 const rateLimiter = new RateLimiter()
 
 /**
- * Lookups that reach the icon providers, per user; icons already cached are not charged. Each
- * lookup can ask the providers about several hosts, and every icon it finds is cached for days.
+ * Lookups that may reach the icon providers, per user; an icon already cached is served without
+ * charge. Each lookup can ask the providers about several hosts, and caches what it finds for days.
  */
 const PROVIDER_LOOKUP_LIMIT: TokenBucketConfig = {
   maxTokens: 120,
@@ -24,12 +24,19 @@ export const getFavicon = defineAuthorizedCredentialUserUseCase({
     principalKinds: ['session'],
     capability: 'none',
   }),
-  execute: ({ principal, input }: { principal: SessionPrincipal; input: { host: string } }) =>
-    resolveFavicon(input.host, async () => {
-      const { allowed } = await rateLimiter.checkRateLimitDirect(
-        `route:favicon-lookup:user:${principal.userId}`,
-        PROVIDER_LOOKUP_LIMIT
-      )
-      return allowed
-    }),
+  async execute({
+    principal,
+    input,
+  }: {
+    principal: SessionPrincipal
+    input: { host: string }
+  }): Promise<FaviconResolution> {
+    const cached = await cachedFavicon(input.host)
+    if (cached) return cached
+    const { allowed } = await rateLimiter.checkRateLimitDirect(
+      `route:favicon-lookup:user:${principal.userId}`,
+      PROVIDER_LOOKUP_LIMIT
+    )
+    return allowed ? resolveFavicon(input.host) : { kind: 'unavailable' }
+  },
 })

apps/sim/lib/favicon/resolve.server.test.ts

@@ -6,7 +6,7 @@ import { createMockRedis } from '@sim/testing/mocks/redis.mock'
 import { redisConfigMockFns } from '@sim/testing/mocks/redis-config.mock'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 import { MAX_ICON_BYTES } from '@/lib/favicon/probe'
-import { resolveFavicon } from '@/lib/favicon/resolve.server'
+import { cachedFavicon, resolveFavicon } from '@/lib/favicon/resolve.server'
 
 vi.mock('@/lib/core/security/input-validation.server', () => inputValidationMock)
 
@@ -73,21 +73,6 @@ describe('resolveFavicon', () => {
     expect(ttlOf('acme.example.com')).toBe(30 * DAY)
   })
 
-  it('asks no provider and caches nothing when a lookup is not admitted', async () => {
-    answers = { 'acme.example.com': 'icon' }
-    expect((await resolveFavicon('acme.example.com', async () => false)).kind).toBe('unavailable')
-    expect(inputValidationMockFns.mockSecureFetchWithValidation).not.toHaveBeenCalled()
-    expect(ttlOf('acme.example.com')).toBeUndefined()
-  })
-
-  it('serves a cached icon without asking to admit a lookup', async () => {
-    answers = { 'acme.example.com': 'icon' }
-    await resolveFavicon('acme.example.com')
-    const admit = vi.fn(async () => false)
-    expect((await resolveFavicon('acme.example.com', admit)).kind).toBe('hit')
-    expect(admit).not.toHaveBeenCalled()
-  })
-
   it('looks up an icon cached under an earlier, larger cap again instead of serving it', async () => {
     answers = { 'acme.example.com': 'icon' }
     const oversized = Buffer.concat([Buffer.from('h'), PNG, Buffer.alloc(MAX_ICON_BYTES)])
@@ -97,37 +82,20 @@ describe('resolveFavicon', () => {
     expect(inputValidationMockFns.mockSecureFetchWithValidation).toHaveBeenCalled()
   })
 
-  it("never lets one caller's refused budget answer another caller's lookup", async () => {
+  it('reports nothing cached until a lookup has stored an answer', async () => {
     answers = { 'acme.example.com': 'icon' }
-    const [refused, admitted] = await Promise.all([
-      resolveFavicon('acme.example.com', async () => false),
-      resolveFavicon('acme.example.com', async () => true),
-    ])
-    expect(refused.kind).toBe('unavailable')
-    expect(admitted.kind).toBe('hit')
+    expect(await cachedFavicon('acme.example.com')).toBeNull()
+    await resolveFavicon('acme.example.com')
+    expect((await cachedFavicon('acme.example.com'))?.kind).toBe('hit')
   })
 
-  it('does not charge a caller who joins a lookup already asking the providers', async () => {
+  it('shares one lookup between concurrent callers of a host', async () => {
     answers = { 'acme.example.com': 'icon' }
-    const fetchProvider = inputValidationMockFns.mockSecureFetchWithValidation
-    const answerProvider = fetchProvider.getMockImplementation()
-    let release = () => {}
-    const released = new Promise<void>((resolve) => {
-      release = resolve
-    })
-    fetchProvider.mockImplementation(
-      async (...args: Parameters<NonNullable<typeof answerProvider>>) => {
-        await released
-        return answerProvider?.(...args)
-      }
-    )
-    const first = resolveFavicon('acme.example.com')
-    await vi.waitFor(() => expect(fetchProvider).toHaveBeenCalled())
-    const joinerAdmit = vi.fn(async () => true)
-    const joined = resolveFavicon('acme.example.com', joinerAdmit)
-    release()
-    expect((await joined).kind).toBe('hit')
-    expect((await first).kind).toBe('hit')
-    expect(joinerAdmit).not.toHaveBeenCalled()
+    const results = await Promise.all([
+      resolveFavicon('acme.example.com'),
+      resolveFavicon('acme.example.com'),
+    ])
+    expect(results.map((result) => result.kind)).toEqual(['hit', 'hit'])
+    expect(inputValidationMockFns.mockSecureFetchWithValidation).toHaveBeenCalledTimes(1)
   })
 })

apps/sim/lib/favicon/resolve.server.ts

@@ -1,4 +1,5 @@
 import { createLogger } from '@sim/logger'
+import { coalesceLocally } from '@/lib/concurrency/singleflight'
 import { getRedisClient } from '@/lib/core/config/redis'
 import { runWithOutboundOrganization } from '@/lib/core/network/context.server'
 import { secureFetchWithValidation } from '@/lib/core/security/input-validation.server'
@@ -34,13 +35,11 @@ const PROVIDERS: readonly ((host: string) => string)[] = [
   (host) => `https://icons.duckduckgo.com/ip3/${host}.ico`,
 ]
 
-type FaviconResolution =
+export type FaviconResolution =
   | { kind: 'hit'; contentType: string; bytes: Buffer; provisional?: true }
   | { kind: 'miss' }
   | { kind: 'unavailable' }
 
-const inFlight = new Map<string, Promise<FaviconResolution>>()
-
 const cacheKey = (host: string) => `favicon:v3:${host}`
 
 /** A host's key holds the result of the walk that starts at that host. */
@@ -152,39 +151,30 @@ async function walk(candidates: string[], signal: AbortSignal): Promise<FaviconR
   return unavailable ? { kind: 'unavailable' } : { kind: 'miss' }
 }
 
+/** The cached answer for a host, or `null` when it would take a lookup to know. */
+export async function cachedFavicon(host: string): Promise<FaviconResolution | null> {
+  const requested = faviconHostCandidates(host)[0]
+  return requested ? readCache(requested) : { kind: 'miss' }
+}
+
 /**
  * The icon for a host, or its nearest parent domain's, from public icon providers — Sim never
  * contacts the destination itself (a provider may, from its own servers), so a host behind a
  * login still gets its platform's icon. Results are cached per host (30 days for an icon, a day
  * for none or for a parent's icon chosen after a timeout, minutes after an outage), and
- * concurrent lookups of one host in this process share a single resolution. `admitLookup` is
- * the caller's own budget, asked only when nothing is cached and no lookup of the host is already
- * running: refused, the host is reported unavailable to this caller alone and nothing is cached,
- * so cached icons and lookups shared with another caller stay free.
+ * concurrent lookups of one host in this process share a single resolution.
  */
-export async function resolveFavicon(
-  host: string,
-  admitLookup: () => Promise<boolean> = async () => true
-): Promise<FaviconResolution> {
+export async function resolveFavicon(host: string): Promise<FaviconResolution> {
   const candidates = faviconHostCandidates(host)
   const requested = candidates[0]
   if (!requested) return { kind: 'miss' }
-  const joined = inFlight.get(requested)
-  if (joined) return joined
-  const cached = await readCache(requested)
-  if (cached) return cached
-  if (!(await admitLookup())) return { kind: 'unavailable' }
-  // Another caller may have started the same lookup while this one read the cache and its budget.
-  const pending = inFlight.get(requested)
-  if (pending) return pending
-
-  const resolution = (async (): Promise<FaviconResolution> => {
+  return coalesceLocally(`favicon:${requested}`, async (): Promise<FaviconResolution> => {
+    const cached = await readCache(requested)
+    if (cached) return cached
     const resolved = await runWithOutboundOrganization(null, () =>
       walk(candidates, AbortSignal.timeout(RESOLVE_DEADLINE_MS))
     ).catch((): FaviconResolution => ({ kind: 'unavailable' }))
     await writeCache(requested, resolved)
     return resolved
-  })().finally(() => inFlight.delete(requested))
-  inFlight.set(requested, resolution)
-  return resolution
+  }).catch((): FaviconResolution => ({ kind: 'unavailable' }))
 }