Skip to content

Commit 04423ff

Browse files
chaxusclaude
andcommitted
fix: apply a second review pass over this branch
Seven things a read-through of the branch turned up. Each is reverse-verified: with the fix removed, the case named for it goes red. - The circular buttons in @ranuts/chat-ui were round only because the SITE zeroes button padding. The rules ship in the package; a host without that reset renders the same 60x50 ellipse this repo already shipped once. They now declare their own box. - @ranuts/converter's loadScript replaced every cause with "Failed to load X2T WASM script". A host classifies open failures on the message text, and that wording matches none of its environment cases -- so a CDN 500 or a refused heap on the buffered path was reported as a possibly-corrupt document, and never retried. The cause is kept in the message, as the vendor loader does. - probeX2tMemory committed x2t's whole 283 MB on a browser that had just refused it, twice per failure (the guard's asc_onError, then the vendor's own -82 for the same failure). Once refused, the answer is reused; a user-initiated open asks again, since the reader may have freed memory. - The landing page refused to promote a waiting worker while ANY second window was open, including another landing tab -- so a reader who keeps two tabs of the site open never got an update. The worker now reports how many windows are on the editor route, which is the only kind an activation can disturb. - wouldDiscardVendorAssets is evaluated during install, and activate deletes the mismatched caches right after. In between, a page of the outgoing build can write its first vendor entries into its own cache. The delete asks again. - Unrelated locale churn, one line of it wrong: Korean particles do not take a space ("API가"), and the new Korean string had picked up the same habit. - VENDOR_VERSION hashed `$DIST_DIR/sdkjs/...`, and shasum prints the path next to the digest -- so the same vendor tree built into dist-e2e-4174/ named a different runtime cache than one built into dist/. Hashed from inside now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 1994c62 commit 04423ff

15 files changed

Lines changed: 434 additions & 51 deletions

‎bin/build.sh‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -126,14 +126,20 @@ if [ -f "$SW_PATH" ]; then
126126
HASH_CMD="sha256sum"
127127
fi
128128
VENDOR_DIRS=""
129-
for dir in "$DIST_DIR/sdkjs" "$DIST_DIR/web-apps" "$DIST_DIR/fonts"; do
130-
if [ -d "$dir" ]; then
129+
for dir in sdkjs web-apps fonts; do
130+
if [ -d "$DIST_DIR/$dir" ]; then
131131
VENDOR_DIRS="$VENDOR_DIRS $dir"
132132
fi
133133
done
134134
if [ -n "$VENDOR_DIRS" ]; then
135+
# Hashed from inside $DIST_DIR, with RELATIVE paths: `shasum` prints the
136+
# path next to each digest, so hashing "$DIST_DIR/sdkjs/..." would fold
137+
# the output directory's own name into the stamp -- the same vendor tree
138+
# built into `dist-e2e-4174/` would name a different runtime cache than
139+
# the one built into `dist/`. What this stamp claims to be, and what
140+
# sw.js relies on, is the content of the tree.
135141
# shellcheck disable=SC2086 # deliberate word splitting: a list of dirs
136-
VENDOR_VERSION=$(find $VENDOR_DIRS -type f -exec $HASH_CMD {} + | LC_ALL=C sort | $HASH_CMD | cut -c1-12)
142+
VENDOR_VERSION=$(cd "$DIST_DIR" && find $VENDOR_DIRS -type f -exec $HASH_CMD {} + | LC_ALL=C sort | $HASH_CMD | cut -c1-12)
137143
else
138144
echo "[build] Warning: no vendor tree found; sw.js keeps its dev vendor stamp." >&2
139145
VENDOR_VERSION="novendor"
Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
# Review 第二轮:#144 那批改动的七条追补(2026-08-20)
2+
3+
第一轮追补见
4+
[2026-08-20-review-followups.md](2026-08-20-review-followups.md);那之后又对整个
5+
分支通读了一遍,记下这七条。CDN 抖动那条单独成文
6+
([2026-08-20-x2t-wasm-fetch-transient-retry.md](2026-08-20-x2t-wasm-fetch-transient-retry.md)),
7+
这里是其余六条 + 一条纯文案回退。
8+
9+
## 1. 圆形按钮靠宿主页的 reset 撑着,包自己不自足
10+
11+
拆掉 Tailwind 后补的 `button { padding: 0; box-sizing: border-box }` 修好了
12+
`.agent-launcher` 的 60×50 椭圆。但同样形状的 `.cui-send` /
13+
`.cui-scroll-bottom` 住在 `@ranuts/chat-ui` 里——它们「定宽高 32px +
14+
border-radius: 50%」,圆不圆取决于**宿主页**有没有那条 reset。生态里另一个站点
15+
引这个包、没有同样的 reset,就会原样复现同一个椭圆,而且没有任何东西会提醒它。
16+
17+
修法:把 `box-sizing: border-box; padding: 0` 写进这两条规则自身。本仓的全局
18+
reset 保留,作为页面级兜底(`.agent-launcher` 等本仓元素仍然靠它)。
19+
20+
## 2. `@ranuts/converter` 的 `loadScript` 把失败原因吞了
21+
22+
```ts
23+
} catch (error) {
24+
const errorMsg = 'Failed to load X2T WASM script';
25+
console.error(errorMsg, error);
26+
throw new Error(errorMsg); // ← 原因没了
27+
}
28+
```
29+
30+
这次改动刚刚立下的契约是「失败消息要带得动原因,宿主的
31+
`classifyOpenFailure` 按消息文本分类」。而 `Failed to load X2T WASM script`
32+
一个关键词都不匹配,落到默认分支 `document`——于是 buffered 兜底路径上的
33+
CDN 500 或者内存拒绝,会被当成「文件可能已损坏」报给用户,还不重试。
34+
vendor 那份 helper 是原样重抛的,两份实现说好语义一致,这里破了。
35+
36+
修法:`throw new Error(\`Failed to load X2T WASM script: ${cause}\`, { cause: error })`。
37+
38+
## 3. 内存诊断探测自己要 283 MB,而且一次失败会探两遍
39+
40+
`probeX2tMemory` 的 commit 半段是真向浏览器提交 x2t 的整个堆——跑在一台刚刚
41+
拒绝过这笔分配的机器上。`skipCommit` 只挡住「重开在飞」那条路,而一次失败的
42+
打开**会两次走到 toast**(守卫把 rejection 送进 `asc_onError` 一次,vendor 自己
43+
再为同一个失败报一次 -82),所以最终报错那一刻会连着要两次 283 MB。
44+
45+
修法:浏览器一旦拒绝过 commit,本 session 内不再问(`commitRefused`),直接复用
46+
那个结论;`registerOpenAttempt` 在用户发起新的一次打开时清掉它——那是新情况,
47+
用户可能刚照提示关了几个标签页,值得重新问。
48+
49+
## 4. 「只要还有第二个窗口就不提升 worker」太粗
50+
51+
落地页提升等待中的 worker 前会问 active worker「你控制着几个窗口」,只有回答
52+
1 才提升。可它拒绝的其实是**任何**第二个窗口,包括另一个落地页标签——而落地页
53+
没有任何会被激活毁掉的会话。习惯常开两个本站标签页的人,因此永远拿不到新版本。
54+
55+
worker 侧本来就拿得到 `client.url`。现在回答里多带一个 `editors`(路径匹配
56+
`/editor` 或 `/editor.html` 的窗口数),落地页只在 `editors === 0` 时提升;
57+
是否开着文档仍然不可知,所以任何编辑器窗口都算拦。旧 worker 的回答里没有这个
58+
字段,那种情况回落到原来的「我是不是唯一窗口」。
59+
60+
## 5. install 的判据与 activate 之间有一道缝
61+
62+
`wouldDiscardVendorAssets` 在 **install** 求值,`activate` 紧接着就删掉名字不
63+
匹配的 runtime cache。这中间旧构建的页面完全可能刚把第一批 vendor 条目写进
64+
它自己的 cache——install 那一刻看到的是空的,于是放行;到 activate 就把它删了,
65+
正是这条判据要防的混版。
66+
67+
修法:删之前再问一次(`holdsVendorAssetsForOpenWindow`)。代价是每个陈旧
68+
cache 一次 `keys()`,把窗口缩到「这次读」和「这次删」之间。没有窗口打开时照删
69+
不误——那时没人会被伤到。
70+
71+
## 6. i18n 里混进了与本次改动无关的润色,韩语那条还改错了
72+
73+
`API가` 被改成 `API 가`(韩语助词不空格),新加的韩语文案也带同样的
74+
「括号后加空格」习惯。日语几处 `: `→`:` 无害但同样无关。全部回退/修正,
75+
新韩语文案改成 `메모리(약 {mb} MB)를` / `64비트`。
76+
77+
## 7. `VENDOR_VERSION` 其实是「路径 + 内容」哈希
78+
79+
`shasum` 会把路径打印在摘要旁边,而 `find "$DIST_DIR/sdkjs" ...` 传的是带
80+
`$DIST_DIR` 前缀的路径——于是同一棵 vendor 树,构建到 `dist/` 和构建到
81+
`dist-e2e-4174/` 会得到两个不同的 runtime cache 名。生产只有一个 DIST_DIR,
82+
所以不影响线上,但注释和 CLAUDE.md 都写的是「内容哈希」。改成
83+
`cd "$DIST_DIR" && find sdkjs web-apps fonts ...`,名副其实。
84+
85+
## 用例与反向验证
86+
87+
| 修复 | 用例 | 去掉修复后 |
88+
| ---- | --------------------------------------------------------------------------------------- | ------------- |
89+
| 1 | `styles-contract`:包内两条规则各自声明 border-box + padding 0 | 2 red |
90+
| 2 | `converter-wasm-loading`:buffered 路径 404,`loadScript` 的消息要带原文 | 1 red |
91+
| 3 | `onlyoffice-wasm-memory`:拒绝过就不再问;`resetMemoryProbe` 后再问 | 1 red |
92+
| 4 | `sw-register`:editors>0 拦、第二个落地页照提升、旧回答回落;sw.js 源钉 + `sw-warm` E2E | 1 red(源钉) |
93+
| 5 | `sw-update`:陈旧 cache 有 vendor 条目且有窗口时不删;没窗口照删;core cache 照删 | 1 red |
94+
| 7 | `sw-update`:`VENDOR_VERSION=$(cd "$DIST_DIR" && find …)` | 1 red |
95+
96+
第 4 条的页面侧行为(第二个落地页照提升)用假 controller 测,去掉 sw.js 那半不会
97+
让它变红——变红的是钉 sw.js 源的那条,加上 `sw-warm` 里对真实 worker 回答
98+
`{count, editors}` 的断言。第 6 条是纯文案回退,没有用例。

‎lib/onlyoffice/open-failure.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { t } from '@ranuts/shared/i18n';
22
import { getDocumentOpenError, isDocumentContentReady, markDocumentOpenFailed, noteFrameError } from './open-state';
33
import { failPendingSaveConversion } from './save-stream';
4-
import { isWasmAllocationFailure } from './wasm-memory';
4+
import { isWasmAllocationFailure, resetMemoryProbe } from './wasm-memory';
55

66
/**
77
* Everything about an open that did not survive: which document was being
@@ -257,6 +257,9 @@ export function registerOpenAttempt(config: {
257257
if (config.isRetry) return;
258258
// A user-initiated open supersedes whatever the last attempt was doing.
259259
openRetryInFlight = false;
260+
// And it is worth asking the browser about memory again: the reader may have
261+
// closed the tabs the last failure told them to close (see resetMemoryProbe).
262+
resetMemoryProbe();
260263
currentOpenAttempt = {
261264
fileName: config.fileName,
262265
fileType: config.fileType,

‎lib/onlyoffice/wasm-memory.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,27 @@ export enum WasmMemoryVerdict {
9393
Unavailable = 'unavailable',
9494
}
9595

96+
/**
97+
* Whether this browser has already refused to commit x2t's heap.
98+
*
99+
* A single failed open reaches the toast more than once -- the guard routes
100+
* the rejection into `asc_onError` and the vendor raises its own -82 for the
101+
* same failure -- and each pass would otherwise ask the browser for another
102+
* 283 MB it has just said it does not have. Once refused, the answer is
103+
* reused: it is the same answer, and asking again costs exactly the memory
104+
* that is missing.
105+
*
106+
* Reset by `registerOpenAttempt` for a user-initiated open (open-failure.ts),
107+
* which is a new situation and worth asking about again -- the reader may have
108+
* closed the tabs the last toast told them to close.
109+
*/
110+
let commitRefused = false;
111+
112+
/** See `commitRefused`: a user-initiated open asks the browser afresh. */
113+
export function resetMemoryProbe(): void {
114+
commitRefused = false;
115+
}
116+
96117
/**
97118
* `skipCommit` leaves the commit half unasked and answers `Deferred`.
98119
*
@@ -119,6 +140,7 @@ export function probeX2tMemory(options?: { skipCommit?: boolean }): WasmMemoryVe
119140
// it is the half GitHub #144 points at; the commit half is worth 283 MB only
120141
// when nothing else needs them.
121142
if (options?.skipCommit) return WasmMemoryVerdict.Deferred;
143+
if (commitRefused) return WasmMemoryVerdict.Commit;
122144
try {
123145
// x2t's descriptor verbatim, `maximum` included. Dropping it would make
124146
// this a question the engine is never actually asked: the two halves can
@@ -128,6 +150,7 @@ export function probeX2tMemory(options?: { skipCommit?: boolean }): WasmMemoryVe
128150
// failure that repeats every time. Costs the same 283 MB either way.
129151
void new Memory({ initial: X2T_INITIAL_PAGES, maximum: X2T_MAXIMUM_PAGES });
130152
} catch {
153+
commitRefused = true;
131154
return WasmMemoryVerdict.Commit;
132155
}
133156
return WasmMemoryVerdict.Ok;

‎packages/chat-ui/src/styles.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,13 @@ export const CHAT_UI_CSS = `
154154
transform: translateX(-50%);
155155
width: 32px;
156156
height: 32px;
157+
/* A round button is round only if its 32px includes the UA's own 1px 6px
158+
button padding and this 1px border. Declared here rather than left to a
159+
host reset: this file ships in @ranuts/chat-ui, and a host without one
160+
renders these as ellipses (which is exactly what happened to the site's
161+
own launcher when its CSS framework went away). */
162+
box-sizing: border-box;
163+
padding: 0;
157164
display: flex;
158165
align-items: center;
159166
justify-content: center;
@@ -218,6 +225,9 @@ export const CHAT_UI_CSS = `
218225
flex: 0 0 auto;
219226
width: 32px;
220227
height: 32px;
228+
/* Same reason as .cui-scroll-bottom: the 32px circle owns its own box. */
229+
box-sizing: border-box;
230+
padding: 0;
221231
display: flex;
222232
align-items: center;
223233
justify-content: center;

‎packages/converter/src/document-converter.ts‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -291,9 +291,18 @@ export class X2TConverter {
291291
this.hasScriptLoaded = true;
292292
console.log('X2T WASM script loaded successfully');
293293
} catch (error) {
294-
const errorMsg = 'Failed to load X2T WASM script';
295-
console.error(errorMsg, error);
296-
throw new Error(errorMsg);
294+
// Keep the cause in the message, not just in `cause`. What reads this is
295+
// a host's open-failure handling (lib/onlyoffice/open-failure.ts
296+
// classifies on the message text), and a bare "Failed to load X2T WASM
297+
// script" matches none of its environment wordings -- so a CDN 500 or a
298+
// refused wasm heap on the buffered path, neither of which is a verdict
299+
// on the document, was reported to the user as "the file may be
300+
// corrupted" and never retried. The vendor-side loader
301+
// (public/sdkjs/common/wasm/x2t/x2t_helper.js) rethrows its cause
302+
// untouched; this is the same contract.
303+
const cause = error instanceof Error ? error.message : String(error);
304+
console.error('Failed to load X2T WASM script', error);
305+
throw new Error(`Failed to load X2T WASM script: ${cause}`, { cause: error });
297306
}
298307
}
299308

‎packages/shared/src/i18n.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -351,12 +351,12 @@ const partialMessages: Record<Exclude<Language, LanguageCode.ZH | LanguageCode.E
351351
themeSystem: 'システム',
352352
themeLight: 'ライト',
353353
themeDark: 'ダーク',
354-
fileSavedSuccess: 'ファイルを保存しました:',
355-
documentLoaded: 'ドキュメントを読み込みました:',
354+
fileSavedSuccess: 'ファイルを保存しました: ',
355+
documentLoaded: 'ドキュメントを読み込みました: ',
356356
failedToLoadEditor: 'エディターを読み込めませんでした。OnlyOffice API が正しく配置されているか確認してください。',
357-
unsupportedFileType: 'サポートされていないファイル形式:',
357+
unsupportedFileType: 'サポートされていないファイル形式: ',
358358
invalidFileObject: '無効なファイルです',
359-
documentOperationFailed: 'ドキュメントの操作に失敗しました:',
359+
documentOperationFailed: 'ドキュメントの操作に失敗しました: ',
360360
editorErrorToast: 'ドキュメントエラー',
361361
editorErrorFormatMismatch: 'ファイルの内容が拡張子と一致しません。形式を確認してからもう一度お試しください',
362362
editorErrorOpenFailed:
@@ -376,7 +376,7 @@ const partialMessages: Record<Exclude<Language, LanguageCode.ZH | LanguageCode.E
376376
themeDark: '다크',
377377
fileSavedSuccess: '파일을 저장했습니다: ',
378378
documentLoaded: '문서를 불러왔습니다: ',
379-
failedToLoadEditor: '편집기를 불러오지 못했습니다. OnlyOffice API 가 올바르게 설치되어 있는지 확인하세요.',
379+
failedToLoadEditor: '편집기를 불러오지 못했습니다. OnlyOffice API가 올바르게 설치되어 있는지 확인하세요.',
380380
unsupportedFileType: '지원하지 않는 파일 형식: ',
381381
invalidFileObject: '잘못된 파일입니다',
382382
documentOperationFailed: '문서 작업에 실패했습니다: ',
@@ -385,7 +385,7 @@ const partialMessages: Record<Exclude<Language, LanguageCode.ZH | LanguageCode.E
385385
editorErrorOpenFailed:
386386
'파일을 열 수 없습니다. 손상되었거나, 지원하지 않는 형식이거나, 확장자와 내용이 다를 수 있습니다',
387387
editorErrorOutOfMemory:
388-
'이 브라우저에서 문서 변환 엔진에 필요한 메모리 (약 {mb} MB) 를 할당할 수 없습니다. 다른 탭이나 창을 닫고 다시 시도하세요. 계속 실패하면 64 비트 브라우저 (Edge 또는 64 비트 Chrome) 를 사용하세요.',
388+
'이 브라우저에서 문서 변환 엔진에 필요한 메모리(약 {mb} MB)를 할당할 수 없습니다. 다른 탭이나 창을 닫고 다시 시도하세요. 계속 실패하면 64비트 브라우저(Edge 또는 64비트 Chrome)를 사용하세요.',
389389
},
390390
[LanguageCode.DE]: {
391391
webOffice: 'Web Office',

‎public/sw-register.js‎

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,11 @@
2020
* This is the missing half, and the landing page is the right place for it:
2121
* it has no document, so promoting here cannot corrupt a session of its own.
2222
* The one case it must still refuse is another tab that does have a document
23-
* open, which the page cannot see -- so it asks the active worker how many
24-
* window clients it controls and only promotes when it is alone.
23+
* open, which the page cannot see -- so it asks the active worker what it
24+
* controls and only promotes when no window is on the editor route. Another
25+
* landing tab is not a reason to refuse: it has nothing to lose, and treating
26+
* every second window as a possible editor left a reader who keeps two tabs of
27+
* the site open on an old build for good.
2528
*
2629
* Plain JS on purpose: the landing pages ship no bundle. `__createSwUpdater`
2730
* is exposed so test/unit/sw-register.test.ts drives this file rather than a
@@ -39,7 +42,7 @@
3942
var timeoutMs = (options && options.timeoutMs) || CLIENT_COUNT_TIMEOUT_MS;
4043

4144
/**
42-
* How many window clients the active worker controls. Resolves to null
45+
* What the active worker controls: `{ count, editors }`. Resolves to null
4346
* when there is nobody to ask or the answer does not arrive -- callers
4447
* treat that as "unknown" and leave the worker waiting.
4548
*/
@@ -59,7 +62,7 @@
5962
};
6063
channel.port1.onmessage = function (event) {
6164
var data = event.data;
62-
done(data && typeof data.count === 'number' ? data.count : null);
65+
done(data && typeof data.count === 'number' ? data : null);
6366
};
6467
global.setTimeout(function () {
6568
done(null);
@@ -73,15 +76,22 @@
7376
}
7477

7578
/**
76-
* Promote the waiting worker, but only while this tab is the only one:
77-
* activation wipes the outgoing build's caches, and another tab could be
78-
* an editor with a document open.
79+
* Promote the waiting worker, but not while another tab is on the editor
80+
* route: activation wipes the outgoing build's caches, and such a tab may
81+
* have a document open and is deliberately not reloaded when the new
82+
* worker takes control.
83+
*
84+
* `editors` is missing from the reply of a worker deployed before it
85+
* existed; there the old, blunter reading applies -- promote only when
86+
* this tab is the only window at all.
7987
*/
8088
function maybePromote(registration) {
8189
var waiting = registration.waiting;
8290
if (!waiting) return Promise.resolve(false);
83-
return countClients().then(function (count) {
84-
if (count === null || count > 1) return false;
91+
return countClients().then(function (answer) {
92+
if (!answer) return false;
93+
var blocked = typeof answer.editors === 'number' ? answer.editors > 0 : answer.count > 1;
94+
if (blocked) return false;
8595
waiting.postMessage({ type: 'SKIP_WAITING' });
8696
return true;
8797
});

0 commit comments

Comments
 (0)