Skip to content

refactor: 代码质量审查修复 + cloud.umami.is API 对齐#2

Merged
yCENzh merged 1 commit into
mainfrom
dev
May 3, 2026
Merged

refactor: 代码质量审查修复 + cloud.umami.is API 对齐#2
yCENzh merged 1 commit into
mainfrom
dev

Conversation

@yCENzh

@yCENzh yCENzh commented May 3, 2026

Copy link
Copy Markdown
Owner

Summary

本 PR 分两批改动:

Phase 1 — 代码质量审查(已合并到本分支的前几次 commit)

  • 修复 3 个 bug:
    • api.tsgetStats 丢弃 url 参数 → getPageStatsByUrl 实际打的是站点级请求。
    • runtime/client.ts SimpleCache:localStorage 既不在加载时恢复到内存,又在保存时被覆盖,刷新后缓存完全失效。
    • astro/integration.ts:读不到 runtime/client.global.js 时仍注入引用 __oddmiscRuntime 的脚本会抛 ReferenceError
  • 类型与契约:CacheManager<T = unknown>UmamiClient 构造异常改抛 UmamiUrlErrorStatsResult.visits 改必填、tsconfig 补 5 条严格选项。
  • 工程与文档:重写 README、补 package.json.description、新增 LICENSE、新增 .github/workflows/ci.yml

Phase 2 — cloud.umami.is 真实逆向对齐
用 Playwright + CDP 连到浏览器抓 share/HM88wjbqInOwBNz0 的 F12 网络请求,比对实际请求头和响应,发现一个关键兼容性 bug:

发现 当前实现 后果
所有 share token 请求都带 x-umami-share-context: 1 只发了 x-umami-share-token 在 cloud.umami.is 与新版自托管上,所有 /websites/* 请求返回 401 Unauthorized
stats 响应携带 bounces / totaltime / comparison 只取 pageviews/visitors/visits 丢弃有价值数据
/active / /pageviews / /metrics / /daterange / /websites/<id> 等端点存在 未暴露 使用者只能拿基础聚合
type=url / type=host 的 metrics 返回 400 类型未约束 易误用

curl 复现验证:

GET /api/websites/<id>/stats                               → 401 (缺 x-umami-share-context)
GET /api/websites/<id>/stats + x-umami-share-context: 1    → 200

Phase 2 改动:

  • api.ts / runtime/client.ts:所有 share 鉴权请求自动附带 x-umami-share-context: 1
  • api.ts:抽出 authedGet 辅助;401 时同时清空 sharePromise,避免复用陈旧 token。
  • 新增 UmamiClient 方法:getActiveVisitors / getPageviews / getMetrics / getWebsite / getDateRange
  • StatsResult 新增可选字段 bounces / totaltime / comparison
  • types.ts 新增 MetricType / MetricEntry / PageviewsSeries / WebsiteInfo / DateRangeindex.ts 对外导出。
  • runtime/client.ts 同步新增 getActiveVisitorswindow.oddmisc 暴露。
  • 测试 27 → 38(新增 context 头校验、401 后 sharePromise 重置、5 个新端点的 URL & 响应契约、StatsResult 新字段透传)。

没改name / version / homepage / repository.url 保持与已发布 npm 包一致。

Review & Testing Checklist for Human

  • 确认 x-umami-share-context: 1 的值在你自托管 Umami 上不会引起副作用(新版代码接受该头;旧版会忽略,但若你的实例有自定义中间件请确认)。
  • 回归自己现有的 getPageStats / getSiteStats / getPageStatsByUrl 调用:返回结构依然兼容,只是多了可选字段。
  • 本地拉下来跑一遍实接入:const c = createUmamiClient({ shareUrl: '你的 share url' }); console.log(await c.getSiteStats(), await c.getActiveVisitors(), await c.getMetrics('path', { limit: 10 })),观察是否按预期返回数据。
  • Astro 集成:npm run dev 一个使用 umami({ shareUrl }) 的站点,在浏览器控制台里 await window.oddmisc.getSiteStats() 应该不再抛 401。

Notes

  • 本地验证:pnpm test(38/38)、pnpm exec tsc --noEmit(无 error)、pnpm build(CJS/ESM/IIFE/.d.ts 齐全)。
  • Phase 2 的 context 头发现对应 Umami 服务端 x-umami-share-context 中间件shareToken 解析分支,此头会在该中间件里把 JWT token 放到 auth.shareToken 上;缺失时 checkAuth 直接判未授权。
  • type=url / type=host 没有放进 MetricType 是因为 cloud.umami.is 明确返回 400,path 已经覆盖 URL 聚合场景。

Link to Devin session: https://app.devin.ai/sessions/5daa18325127407c9e16b867e408fa16
Requested by: @3142979848


Open in Devin Review

@yCENzh
yCENzh merged commit 294c795 into main May 3, 2026
1 check passed

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

View 4 additional findings in Devin Review.

Open in Devin Review

Comment thread src/modules/umami/api.ts
Comment on lines +117 to +134
private buildRangeQuery(range: TimeRange = {}): URLSearchParams {
return new URLSearchParams({
startAt: (range.startAt ?? 0).toString(),
endAt: (range.endAt ?? Date.now()).toString()
});
}

async getStats(baseUrl: string, shareId: string, params: StatsAPIParams): Promise<StatsAPIResponse> {
const { websiteId } = await this.getShareData(baseUrl, shareId);
const qp = this.buildRangeQuery(params);
if (params.path) qp.set('path', params.path);
if (params.url) qp.set('url', params.url);
return this.cachedGet<StatsAPIResponse>(
baseUrl,
shareId,
`/websites/${websiteId}/stats?${qp.toString()}`,
`${baseUrl}|${shareId}|stats|${qp.toString()}`
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Cache key includes Date.now(), making caching ineffective for default time-range queries

When endAt is not explicitly provided by the caller, buildRangeQuery() at src/modules/umami/api.ts:120 fills in Date.now(), and the resulting query-parameter string (which includes this ever-changing timestamp) is embedded in the cache key (lines 133, 176, 190). Because Date.now() returns a different value on every call, the cache key is unique each time, so cachedGet / cacheManager.get will never find a hit.

The old code used JSON.stringify(params) as the cache key (which did not include Date.now()), while Date.now() was only placed in the URL sent to the server. This regression completely defeats the "内存 + localStorage 双级缓存" feature advertised in the README for the most common usage patterns (getSiteStats(), getPageStats('/path'), getPageviews(), getMetrics('path')) — any call without explicit startAt/endAt will always miss the cache and make a network request.

Prompt for agents
The cache key for getStats, getPageviews, and getMetrics includes the full URL query-parameter string produced by buildRangeQuery(), which embeds Date.now() when endAt is not provided. This makes the cache key unique on every call, so the cache is never hit.

The fix should separate the cache key from the URL query parameters. For the cache key, use only the caller-supplied parameters (like the old JSON.stringify(params) approach), while continuing to use the Date.now()-based value in the URL sent to the API server.

Affected methods:
- getStats (line 124-135 in api.ts): cache key on line 133 uses qp.toString()
- getPageviews (line 163-178 in api.ts): cache key on line 176 uses qp.toString()
- getMetrics (line 180-203 in api.ts): cache key on line 190 uses qp.toString()

One approach is to compute the cache key from the user-supplied params before filling in defaults, e.g. building a separate stable key string. Another approach is to use a snapshot of the query params before Date.now() is injected.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@yCENzh
yCENzh deleted the dev branch May 5, 2026 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant