fix: P0/P1 SSR double-end, Electron-safe router, store tearing - #12
Conversation
- ssr/renderToStream: pipe+writable.end() 双重结束会抛 ERR_STREAM_WRITE_AFTER_END，
afterContent (</div></body></html>) 永远写不出去。插入 PassThrough 桥接，用
bridge.pipe(writable, { end: false }) 保留 backpressure，bridge 'end' 再手动写
afterContent + writable.end()
- router: 仅 re-export BrowserRouter 作为 ChenRouter，Electron 打包渲染进程走
file:// 时刷新/深链会 404。新增 ChenHashRouter 导出与 ChenAdaptiveRouter 工厂
(mode: 'browser' | 'hash' | 'auto')，默认 auto 在 file: 协议下自动切 hash
- hooks/use-store:
* createStore 旧实现 useReducer + stateRef (render 阶段写) + useEffect 通知，
并发模式下订阅者看到的状态与 React commit 时机错位，会产生 tearing。
改为每个 Provider 挂一份 makeExternalStore —— state 完全脱离 React render
* useStoreSelector / useGlobalStore(selector) / createSimpleStore useStore(selector)
都缓存 lastRef，getSnapshot 在相等语义下返回稳定引用，消除
"getSnapshot should be cached" 警告与潜在死循环
* useGlobalStore / useGlobalStoreDispatch 增加 selector 重载
62/62 vitest 通过，tsc 干净。
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughReplaces React-managed store internals with per-provider closure-based external stores supporting selectors and equality functions; adds an adaptive router ( Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller\nWritable
participant React as React\nrenderToPipeableStream
participant Bridge as PassThrough\nBridge Stream
participant Writable as CallerWritable\n(writable)
participant Options as Options\n(onAllReady/onError)
Caller->>React: renderToStream(writable, options)
React->>Bridge: pipe rendered chunks into Bridge
Bridge->>Writable: forward chunks to caller writable
React->>React: onAllReady (do not write afterContent)
Bridge->>Bridge: emit "end"
Bridge->>Writable: write shell.afterContent
Bridge->>Writable: end writable
Bridge->>Options: invoke onAllReady()
Bridge->>Options: on error -> onError(err) & destroy Writable
Writable->>Options: on error -> onError(err) & destroy Bridge
Caller->>Writable: call __chenAbort -> React.abort called
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/ssr/index.ts (1)
115-116: Consider a cleaner API for exposingabort.Attaching
__chenAbortas an untyped property works but requires callers to know about this undocumented extension. Consider returning an object with the abort function, or documenting this in the types.♻️ Alternative: Return abort handle or use documented interface
Option 1 — Return an object (breaking change):
export function renderToStream( element: ReactElement, writable: Writable, options: RenderToStreamOptions, ): { abort: () => void } { // ... existing logic ... return { abort }; }Option 2 — Document the property in a type:
export interface ChenWritable extends Writable { __chenAbort?: () => void; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ssr/index.ts` around lines 115 - 116, Replace the ad-hoc untyped cast for __chenAbort with a documented type: declare and export an interface ChenWritable extends Writable { __chenAbort?: () => void } and change the renderToStream signature to accept writable: ChenWritable (or cast to ChenWritable internally) so setting (writable.__chenAbort = abort) is typed; ensure the exported type name ChenWritable and the abort function name are used so callers can rely on the documented API instead of the magic __chenAbort property.src/hooks/use-store.tsx (1)
122-136: Consider extracting shared selector-caching logic.The same caching pattern (check state identity → check selected equality → update cache) is repeated in
useStoreSelector,createSimpleStore.useStore, anduseGlobalStore. After fixing the conditional hooks issue, consider extracting a shared helper to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/use-store.tsx` around lines 122 - 136, Extract the repeated selector-caching logic into a shared helper (e.g., createSelectorCache or useSelectorCache) and replace the duplicated implementations in getSelected, useStoreSelector, createSimpleStore.useStore, and useGlobalStore with calls to that helper; the helper should accept the store.getState (or current state), selector, and equalityFn, maintain an internal lastRef { state, selected } cache, perform the identity check (lastRef.current.state === state), equality check (equalityFn(lastRef.current.selected, next)), and update/return the cached selected value accordingly so all four locations reuse the same deterministic caching logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/use-store.tsx`:
- Around line 209-235: The hooks (useRef and useCallback) must be called
unconditionally to avoid violating React's rules of hooks: move the useRef
initialization (lastRef) and the useCallback definition (getSelected) above the
if (!selector) branch, and inside getSelected guard on selector before calling
it (or use a default identity selector) so the callback is safe when selector is
undefined; then keep the early return for the no-selector case using
useSyncExternalStore(store.subscribe, store.getState, store.getState) and for
the selector case return useSyncExternalStore(store.subscribe, getSelected,
getSelected) — referencing useGlobalStore, lastRef (useRef), getSelected
(useCallback), selector, and useSyncExternalStore.
- Around line 157-180: The hook calls inside useStore must be executed
unconditionally to satisfy React's rules of hooks: move the useRef (lastRef) and
useCallback (getSelected) declarations so they run on every render before the
early return; make getSelected handle the case where selector is undefined by
returning the full store.getState() (typed to R) when no selector is provided;
then keep the early return that returns useSyncExternalStore(store.subscribe,
store.getState, store.getState) or call useSyncExternalStore(store.subscribe,
getSelected, getSelected) consistently, ensuring the order of hook calls
(useRef/useCallback/useSyncExternalStore) is identical regardless of whether
selector is passed.
In `@src/ssr/index.ts`:
- Around line 90-98: The bridge end handler can throw if writable was already
destroyed and writable may emit errors itself; update the bridge.on('end')
handler to first check writable.destroyed or writable.writableEnded before
calling writable.write/shell.afterContent and wrap the write/end in a try/catch
that calls options.onError?.(err) and ensures writable.destroy(err) if the write
fails, and add a writable.on('error', (err) => { options.onError?.(err);
bridge.destroy?.(err) }) handler to propagate independent writable errors back
to the bridge cleanup logic; reference the existing bridge.on('end'),
bridge.on('error'), writable.write, writable.end, writable.destroy, and
options.onError/onAllReady to locate where to apply these changes.
---
Nitpick comments:
In `@src/hooks/use-store.tsx`:
- Around line 122-136: Extract the repeated selector-caching logic into a shared
helper (e.g., createSelectorCache or useSelectorCache) and replace the
duplicated implementations in getSelected, useStoreSelector,
createSimpleStore.useStore, and useGlobalStore with calls to that helper; the
helper should accept the store.getState (or current state), selector, and
equalityFn, maintain an internal lastRef { state, selected } cache, perform the
identity check (lastRef.current.state === state), equality check
(equalityFn(lastRef.current.selected, next)), and update/return the cached
selected value accordingly so all four locations reuse the same deterministic
caching logic.
In `@src/ssr/index.ts`:
- Around line 115-116: Replace the ad-hoc untyped cast for __chenAbort with a
documented type: declare and export an interface ChenWritable extends Writable {
__chenAbort?: () => void } and change the renderToStream signature to accept
writable: ChenWritable (or cast to ChenWritable internally) so setting
(writable.__chenAbort = abort) is typed; ensure the exported type name
ChenWritable and the abort function name are used so callers can rely on the
documented API instead of the magic __chenAbort property.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 918693bd-1c08-4f20-9884-29040790b725
📒 Files selected for processing (3)
src/hooks/use-store.tsxsrc/router/index.tssrc/ssr/index.ts
zhangqiling-1103
left a comment
zhangqiling-1103
left a comment
There was a problem hiding this comment.
看起来有大问题
@coderabbitai autofix
|
@coderabbitai autofix |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 3 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/use-store.tsx`:
- Around line 122-138: The cache currently keys only off state identity causing
stale selections when the selector function changes; update the caching shape
used by lastRef in use-store.tsx (and the identical caches in
createSimpleStore.useStore and useGlobalStore) to include the selector identity
(e.g., change lastRef to hold { state: T; selected: R; selector: SelectorType }
| null), then in getSelected compare both lastRef.current.state === state AND
lastRef.current.selector === selector before returning the cached selected; if
the selector differs, recompute next = selector(state) (and still apply
equalityFn to possibly reuse the old selected), and always update lastRef with
the current selector when storing { state, selected, selector } so selector
changes no longer cause stale values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3117abfa-f7cc-4b33-8d0e-d0d6848058a6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/hooks/use-store.tsxsrc/ssr/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ssr/index.ts
|
@coderabbitai autofix |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/hooks/use-store.tsx (1)
155-186: Refactor: dead branch insidegetSelectedand conditionaluseSyncExternalStorecan be unified.Lines 166–169 (
if (!selector) return state as unknown as R) are unreachable because the early return at line 182 short-circuits beforegetSelectedis ever invoked when no selector is provided. While the current code is functionally correct (hook order is preserved across renders since both branches end in exactly oneuseSyncExternalStorecall), the conditional return with a hook call inside also tends to tripeslint-plugin-react-hooksdespite being safe.Unifying into a single snapshot path removes the dead cast and avoids the false-positive lint shape:
♻️ Proposed refactor
function useStore<R>( selector?: (state: T) => R, equalityFn: (a: R, b: R) => boolean = Object.is, ): T | R { - // 闭包缓存，稳定 getSnapshot 引用 - // 必须在所有分支前调用 hooks，满足 Rules of Hooks const lastRef = useRef<{ state: T; selected: R; selector: (state: T) => R } | null>(null); - const getSelected = useCallback((): R => { + const getSnapshot = useCallback((): T | R => { const state = store.getState(); if (!selector) { - // selector 未传时，返回完整 state（需类型断言为 R） - return state as unknown as R; + return state; } if (lastRef.current && lastRef.current.state === state && lastRef.current.selector === selector) { return lastRef.current.selected; } const next = selector(state); if (lastRef.current && equalityFn(lastRef.current.selected, next)) { lastRef.current = { state, selected: lastRef.current.selected, selector }; return lastRef.current.selected; } lastRef.current = { state, selected: next, selector }; return next; }, [selector, equalityFn]); - if (!selector) { - return useSyncExternalStore(store.subscribe, store.getState, store.getState); - } - return useSyncExternalStore(store.subscribe, getSelected, getSelected); + return useSyncExternalStore(store.subscribe, getSnapshot, getSnapshot); }The same simplification applies to
useGlobalStore(lines 215–246).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/use-store.tsx` around lines 155 - 186, The getSelected function contains a dead branch (if (!selector) return state as unknown as R) because when selector is falsy the hook already calls useSyncExternalStore with store.getState directly; to fix, remove that unreachable return and instead make getSelected always return a value by treating a missing selector as the identity selector (i.e., return store.getState() cast to R), then call useSyncExternalStore exactly once with getSelected for both client and server snapshots; apply the same refactor to useGlobalStore (ensure lastRef / selector/equalityFn logic still works when selector is undefined).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/hooks/use-store.tsx`:
- Around line 155-186: The getSelected function contains a dead branch (if
(!selector) return state as unknown as R) because when selector is falsy the
hook already calls useSyncExternalStore with store.getState directly; to fix,
remove that unreachable return and instead make getSelected always return a
value by treating a missing selector as the identity selector (i.e., return
store.getState() cast to R), then call useSyncExternalStore exactly once with
getSelected for both client and server snapshots; apply the same refactor to
useGlobalStore (ensure lastRef / selector/equalityFn logic still works when
selector is undefined).
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Unit tests committed locally. Commit: |
Two tests created separate `renderHook` calls with the same wrapper, which spawned independent `StoreProvider` instances. Dispatch wrote to one store while the selector read from another, so the failing test never observed the update and the passing one stayed green for the wrong reason. Combine the hooks into a single `renderHook` so they share one Provider, and pass `children` via the props object to satisfy `StoreProvider`'s typing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/__tests__/use-store.test.tsx (1)
193-222: Render-count assertion is correct but fragile under StrictMode.The test depends on
useSyncExternalStoreskipping a re-render becausegetSelectedreturns the cachedselectedreference (Object.is) whenequalityFnreports equality — this is consistent with thelastReflogic inuseStoreSelector(per snippet) and should pass deterministically as configured.However, the assertion
expect(renderCount).toBe(initialRenders)is sensitive to renderer settings: ifrenderHookis ever wrapped in<StrictMode>(or a default test setup adds it), the body would execute twice per logical render and the strict equality assertion could flake. Consider a>=/<=style or asserting "no additional renders after dispatch" via a delta captured immediately beforeact(...)— which is already whatinitialRendersdoes, buttoBeLessThanOrEqual(initialRenders)would be more robust to future StrictMode adoption.♻️ Optional tightening
- expect(renderCount).toBe(initialRenders); + // No extra renders after dispatch (robust to StrictMode double-invocation). + expect(renderCount).toBeLessThanOrEqual(initialRenders);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/use-store.test.tsx` around lines 193 - 222, The test's strict render-count assertion can flake under React StrictMode; change the final assertion to allow equal or fewer renders rather than exact equality: after capturing initialRenders, perform the dispatch and then assert renderCount <= initialRenders (or assert that renderCount - initialRenders === 0 using a delta check) so the check in the useStoreSelector test (referencing renderCount, initialRenders, and the dispatch returned by objStore.useStoreDispatch()) is resilient to StrictMode double-invocations.src/__tests__/ssr.test.ts (1)
98-104: Test leaks an in-flight stream.This test calls
renderToStreamand asserts synchronously without awaitingwaitForFinish(writable). The React stream → bridge → writable pipeline keeps running after the test resolves; the eventualwritable.end()and anyonAllReady/error events fire after Vitest has moved on. While this currently doesn't fail, it can produce flaky logs and "operation after test ended" warnings, and any future change that surfaces errors during shell rendering could cause an unhandled rejection in the wrong test.Suggested cleanup
- it('exposes __chenAbort on the writable', () => { + it('exposes __chenAbort on the writable', async () => { const { writable } = collectWritable(); renderToStream(React.createElement('div'), writable, baseOptions); expect(typeof (writable as Writable & { __chenAbort?: () => void }).__chenAbort).toBe('function'); + await waitForFinish(writable); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/ssr.test.ts` around lines 98 - 104, The test starts a streaming render with renderToStream and then asserts synchronously, which leaks an in-flight stream; update the test (the collectWritable/writable usage) to wait for the stream to finish before asserting or teardown by awaiting waitForFinish(writable) (or otherwise ending the writable) after calling renderToStream and before completing the test so the pipeline (renderToStream → bridge → writable) has fully settled and will not emit events after the test ends.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/router.test.tsx`:
- Around line 22-32: The test captures originalProtocol but never restores
window.location, and some tests replace the real Location with a plain object
(via Object.defineProperty(window, 'location', ...)), leaking state; change the
setup to save the entire original location object (e.g., const originalLocation
= window.location in beforeEach) and in afterEach restore it by calling
Object.defineProperty(window, 'location', { value: originalLocation,
configurable: true }) (or otherwise reassign the original Location object) and
keep vi.restoreAllMocks(); update references to originalProtocol to use
originalLocation so any place that replaced window.location is reverted to the
genuine Location instance.
- Around line 52-138: The tests for ChenAdaptiveRouter only assert child
rendering and don't verify whether BrowserRouter or HashRouter was chosen;
update the tests to assert the actual router branch: either (A) mock and spy the
underlying components (BrowserRouter, HashRouter) with vi.mock and assert
BrowserRouter/HashRouter were called or not in each case (refer to tests using
ChenAdaptiveRouter in src/__tests__/router.test.tsx and the test cases labeled
mode="auto"/"hash"/default), or (B) render a small useLocation/useNavigate-aware
child component and assert observable routing differences (e.g.,
window.location.hash populated in hash mode but empty in browser mode) to prove
the correct router was used; pick one approach and replace the current
container.textContent assertions with the appropriate router-specific
expectations.
In `@src/__tests__/ssr.test.ts`:
- Around line 185-218: The test currently asserts writable.destroyed after
explicitly calling writable.destroy, which is tautological; instead, emit an
error on the writable and assert the bridge error path actually invoked the
onError callback and caused bridge destruction. Change the test to spy
on/replace the PassThrough instance's destroy method (or wrap it with a spy),
call writable.emit('error', err) rather than directly destroying it, then await
completion and assert vi.fn onError was calledWith(err) and that the spy for
writable.destroy was called (or writable.destroyed is true as a result of the
bridge error handler). Reference renderToStream, onError, PassThrough/writable
and the bridge destroy path in src/ssr/index.ts when making the change.
- Around line 154-183: The test currently resolves as soon as the writable is
closed/errored and thus can pass without exercising the guard (if
(writable.destroyed || writable.writableEnded) return;) inside the bridge 'end'
handler; change the test to wait for the bridge to finish before asserting by
either (A) making renderToStream return or expose a promise that resolves when
the internal bridge emits 'end' (update renderToStream to resolve after
bridge.on('end') runs) or (B) if you prefer to keep the public API, wait for a
microtask flush plus an explicit signal from the stream end (e.g., wait for
nextTick/setImmediate and for onError/unhandledRejection to settle) before
asserting that onAllReady was not called; target symbols: renderToStream,
onShellReady, onAllReady, onError and the guard using writable.destroyed /
writable.writableEnded in the bridge 'end' handler so the test reliably
exercises that guard.
---
Nitpick comments:
In `@src/__tests__/ssr.test.ts`:
- Around line 98-104: The test starts a streaming render with renderToStream and
then asserts synchronously, which leaks an in-flight stream; update the test
(the collectWritable/writable usage) to wait for the stream to finish before
asserting or teardown by awaiting waitForFinish(writable) (or otherwise ending
the writable) after calling renderToStream and before completing the test so the
pipeline (renderToStream → bridge → writable) has fully settled and will not
emit events after the test ends.
In `@src/__tests__/use-store.test.tsx`:
- Around line 193-222: The test's strict render-count assertion can flake under
React StrictMode; change the final assertion to allow equal or fewer renders
rather than exact equality: after capturing initialRenders, perform the dispatch
and then assert renderCount <= initialRenders (or assert that renderCount -
initialRenders === 0 using a delta check) so the check in the useStoreSelector
test (referencing renderCount, initialRenders, and the dispatch returned by
objStore.useStoreDispatch()) is resilient to StrictMode double-invocations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60f7b9ee-410b-428e-96e0-c3730a6a576f
📒 Files selected for processing (3)
src/__tests__/router.test.tsxsrc/__tests__/ssr.test.tssrc/__tests__/use-store.test.tsx
| let originalProtocol: string; | ||
|
|
||
| beforeEach(() => { | ||
| // Store original location descriptor | ||
| originalProtocol = window.location.protocol; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| // Restore window.location.protocol if it was changed | ||
| }); |
There was a problem hiding this comment.
afterEach doesn't actually restore window.location; originalProtocol is captured but never read.
Object.defineProperty(window, 'location', { value: { ...window.location, protocol }, writable: true, configurable: true }) replaces the real Location instance with a plain object. vi.restoreAllMocks() does not undo this, and the comment on Line 31 ("Restore window.location.protocol if it was changed") is followed by no code. This leaks state into subsequent tests in this file (and any later test in the same worker that touches window.location), making results order-dependent.
🛠️ Suggested fix
describe('ChenAdaptiveRouter', () => {
- let originalProtocol: string;
-
- beforeEach(() => {
- // Store original location descriptor
- originalProtocol = window.location.protocol;
- });
-
- afterEach(() => {
- vi.restoreAllMocks();
- // Restore window.location.protocol if it was changed
- });
+ let originalLocationDescriptor: PropertyDescriptor | undefined;
+
+ beforeEach(() => {
+ originalLocationDescriptor = Object.getOwnPropertyDescriptor(window, 'location');
+ });
+
+ afterEach(() => {
+ if (originalLocationDescriptor) {
+ Object.defineProperty(window, 'location', originalLocationDescriptor);
+ }
+ vi.restoreAllMocks();
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/router.test.tsx` around lines 22 - 32, The test captures
originalProtocol but never restores window.location, and some tests replace the
real Location with a plain object (via Object.defineProperty(window, 'location',
...)), leaking state; change the setup to save the entire original location
object (e.g., const originalLocation = window.location in beforeEach) and in
afterEach restore it by calling Object.defineProperty(window, 'location', {
value: originalLocation, configurable: true }) (or otherwise reassign the
original Location object) and keep vi.restoreAllMocks(); update references to
originalProtocol to use originalLocation so any place that replaced
window.location is reverted to the genuine Location instance.
| it('mode="hash" renders HashRouter content regardless of protocol', () => { | ||
| Object.defineProperty(window, 'location', { | ||
| value: { ...window.location, protocol: 'http:' }, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| const { container } = render( | ||
| React.createElement( | ||
| ChenAdaptiveRouter, | ||
| { mode: 'hash' }, | ||
| React.createElement('div', null, 'hash-content') | ||
| ) | ||
| ); | ||
| expect(container.textContent).toContain('hash-content'); | ||
| }); | ||
|
|
||
| it('mode="auto" with http: protocol uses BrowserRouter', () => { | ||
| Object.defineProperty(window, 'location', { | ||
| value: { ...window.location, protocol: 'http:' }, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| const { container } = render( | ||
| React.createElement( | ||
| ChenAdaptiveRouter, | ||
| { mode: 'auto' }, | ||
| React.createElement('div', null, 'auto-http-content') | ||
| ) | ||
| ); | ||
| expect(container.textContent).toContain('auto-http-content'); | ||
| }); | ||
|
|
||
| it('mode="auto" with file: protocol uses HashRouter', () => { | ||
| Object.defineProperty(window, 'location', { | ||
| value: { ...window.location, protocol: 'file:' }, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| const { container } = render( | ||
| React.createElement( | ||
| ChenAdaptiveRouter, | ||
| { mode: 'auto' }, | ||
| React.createElement('div', null, 'auto-file-content') | ||
| ) | ||
| ); | ||
| expect(container.textContent).toContain('auto-file-content'); | ||
| }); | ||
|
|
||
| it('default mode (auto) with https: protocol uses BrowserRouter', () => { | ||
| Object.defineProperty(window, 'location', { | ||
| value: { ...window.location, protocol: 'https:' }, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| const { container } = render( | ||
| React.createElement( | ||
| ChenAdaptiveRouter, | ||
| {}, | ||
| React.createElement('div', null, 'default-https') | ||
| ) | ||
| ); | ||
| expect(container.textContent).toContain('default-https'); | ||
| }); | ||
|
|
||
| it('resolves to hash when mode is "hash" even on http: protocol', () => { | ||
| Object.defineProperty(window, 'location', { | ||
| value: { ...window.location, protocol: 'http:' }, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| // mode='hash' should always use HashRouter | ||
| // HashRouter uses window.location.hash, BrowserRouter uses window.history | ||
| // We verify it renders correctly — both work, but hash mode uses #-based routing | ||
| const { container } = render( | ||
| React.createElement( | ||
| ChenAdaptiveRouter, | ||
| { mode: 'hash' }, | ||
| React.createElement('span', null, 'forced-hash') | ||
| ) | ||
| ); | ||
| expect(container.textContent).toContain('forced-hash'); | ||
| }); |
There was a problem hiding this comment.
These tests don't actually verify which router was selected.
Every mode="auto"|"hash"|"browser" test asserts only container.textContent.toContain(...), which is identical regardless of whether BrowserRouter or HashRouter was rendered (both unconditionally render their children). As written, ChenAdaptiveRouter could ignore mode entirely or always pick the wrong branch and these tests would still pass — so they don't protect the P0 fix described in the PR (Electron file:// deep-link 404).
To actually exercise the branch decision, assert something observable that differs between routers. Two practical options:
♻️ Option A — spy on the underlying router components via vi.mock
+vi.mock('react-router', async () => {
+ const actual = await vi.importActual<typeof import('react-router')>('react-router');
+ return {
+ ...actual,
+ BrowserRouter: vi.fn((props: any) => actual.BrowserRouter(props)),
+ HashRouter: vi.fn((props: any) => actual.HashRouter(props)),
+ };
+});Then in each test assert e.g. expect(BrowserRouter).toHaveBeenCalled() / expect(HashRouter).not.toHaveBeenCalled().
♻️ Option B — render a useLocation-aware child and assert URL shape
Navigate via Link/useNavigate and assert window.location.hash is non-empty for hash mode and empty for browser mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/router.test.tsx` around lines 52 - 138, The tests for
ChenAdaptiveRouter only assert child rendering and don't verify whether
BrowserRouter or HashRouter was chosen; update the tests to assert the actual
router branch: either (A) mock and spy the underlying components (BrowserRouter,
HashRouter) with vi.mock and assert BrowserRouter/HashRouter were called or not
in each case (refer to tests using ChenAdaptiveRouter in
src/__tests__/router.test.tsx and the test cases labeled
mode="auto"/"hash"/default), or (B) render a small useLocation/useNavigate-aware
child component and assert observable routing differences (e.g.,
window.location.hash populated in hash mode but empty in browser mode) to prove
the correct router was used; pick one approach and replace the current
container.textContent assertions with the appropriate router-specific
expectations.
| it('does not write afterContent if writable is already destroyed before bridge ends', async () => { | ||
| // This tests the guard: if (writable.destroyed || writable.writableEnded) return; | ||
| const onAllReady = vi.fn(); | ||
| const onError = vi.fn(); | ||
|
|
||
| // Create a writable that we'll destroy immediately after shell | ||
| const chunks: string[] = []; | ||
| const writable = new PassThrough(); | ||
| writable.on('data', (d: Buffer) => chunks.push(d.toString())); | ||
|
|
||
| const onShellReady = vi.fn(() => { | ||
| // Destroy the writable right after shell is ready | ||
| writable.destroy(new Error('client disconnected')); | ||
| }); | ||
|
|
||
| await new Promise<void>((resolve) => { | ||
| writable.on('close', resolve); | ||
| writable.on('error', () => resolve()); // swallow the error for test | ||
|
|
||
| renderToStream(React.createElement('div', null, 'content'), writable, { | ||
| ...baseOptions, | ||
| onShellReady, | ||
| onAllReady, | ||
| onError, | ||
| }); | ||
| }); | ||
|
|
||
| // onAllReady should NOT have been called because writable was destroyed | ||
| expect(onAllReady).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Race: this test can pass even without the destroyed/writableEnded guard.
The promise resolves on writable's close/error, which fires immediately after writable.destroy(...) inside onShellReady. At that moment the React → bridge piping is still in progress, so bridge.on('end') (where the guard and onAllReady?.() live) has not run yet. expect(onAllReady).not.toHaveBeenCalled() therefore passes simply because the bridge hasn't ended, regardless of whether the guard is present. Removing if (writable.destroyed || writable.writableEnded) return; from src/ssr/index.ts would not cause this test to fail — it would just throw ERR_STREAM_WRITE_AFTER_END asynchronously after the test has already resolved.
Consider awaiting the bridge end (or a microtask flush) before asserting, e.g. by exposing the bridge or by waiting on a longer tick window plus an onError/unhandledRejection check, so the guard is actually exercised.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/ssr.test.ts` around lines 154 - 183, The test currently
resolves as soon as the writable is closed/errored and thus can pass without
exercising the guard (if (writable.destroyed || writable.writableEnded) return;)
inside the bridge 'end' handler; change the test to wait for the bridge to
finish before asserting by either (A) making renderToStream return or expose a
promise that resolves when the internal bridge emits 'end' (update
renderToStream to resolve after bridge.on('end') runs) or (B) if you prefer to
keep the public API, wait for a microtask flush plus an explicit signal from the
stream end (e.g., wait for nextTick/setImmediate and for
onError/unhandledRejection to settle) before asserting that onAllReady was not
called; target symbols: renderToStream, onShellReady, onAllReady, onError and
the guard using writable.destroyed / writable.writableEnded in the bridge 'end'
handler so the test reliably exercises that guard.
| it('calls onError callback when bridge encounters an error', async () => { | ||
| const onError = vi.fn(); | ||
|
|
||
| const chunks: Buffer[] = []; | ||
| const writable = new PassThrough(); | ||
| writable.on('data', (d: Buffer) => chunks.push(d)); | ||
|
|
||
| // We'll manually emit an error on the writable to test bi-directional error propagation | ||
| let resolved = false; | ||
| const done = new Promise<void>((resolve) => { | ||
| writable.on('close', () => { | ||
| if (!resolved) { resolved = true; resolve(); } | ||
| }); | ||
| writable.on('error', () => { | ||
| if (!resolved) { resolved = true; resolve(); } | ||
| }); | ||
| }); | ||
|
|
||
| renderToStream(React.createElement('div'), writable, { | ||
| ...baseOptions, | ||
| onError, | ||
| }); | ||
|
|
||
| // Emit an error on writable to trigger the error propagation path | ||
| process.nextTick(() => { | ||
| if (!writable.destroyed && !writable.writableEnded) { | ||
| writable.destroy(new Error('upstream error')); | ||
| } | ||
| }); | ||
|
|
||
| await done; | ||
| // onError may or may not be called depending on timing, but writable should be destroyed | ||
| expect(writable.destroyed).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Assertion is tautological — doesn't actually verify bidirectional error propagation.
The test itself calls writable.destroy(new Error('upstream error')) and then asserts expect(writable.destroyed).toBe(true), which is true by construction. Combined with the comment "onError may or may not be called depending on timing", nothing in this test would fail if the writable.on('error', …) → bridge.destroy(err) path in src/ssr/index.ts were removed.
Suggest asserting the actual propagation contract: onError is invoked with the error, and the bridge gets destroyed. For example, capture the bridge via a mocked PassThrough or inject a spy, or at minimum:
Proposed strengthening
- // onError may or may not be called depending on timing, but writable should be destroyed
- expect(writable.destroyed).toBe(true);
+ // Give the error handler a tick to run.
+ await new Promise((r) => setImmediate(r));
+ expect(onError).toHaveBeenCalledTimes(1);
+ expect(onError).toHaveBeenCalledWith(expect.objectContaining({ message: 'upstream error' }));
+ expect(writable.destroyed).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/ssr.test.ts` around lines 185 - 218, The test currently asserts
writable.destroyed after explicitly calling writable.destroy, which is
tautological; instead, emit an error on the writable and assert the bridge error
path actually invoked the onError callback and caused bridge destruction. Change
the test to spy on/replace the PassThrough instance's destroy method (or wrap it
with a spy), call writable.emit('error', err) rather than directly destroying
it, then await completion and assert vi.fn onError was calledWith(err) and that
the spy for writable.destroy was called (or writable.destroyed is true as a
result of the bridge error handler). Reference renderToStream, onError,
PassThrough/writable and the bridge destroy path in src/ssr/index.ts when making
the change.
2 participants
Summary
修复 Logos Workstation 集成时发现的 3 个阻塞问题：
P0
renderToStream双重end()—pipe(writable)默认在 React 流结束时调用writable.end()，随后onAllReady里再write(afterContent) + end()会抛ERR_STREAM_WRITE_AFTER_END，</div></body></html>永远写不出去。插PassThrough桥接，bridge.pipe(writable, { end: false })保留 backpressure，bridge'end'再手动写 afterContent。BrowserRouter— 打包渲染进程通过file://加载index.html，BrowserRouter刷新/深链 404（无 SPA fallback）。新增ChenHashRouter与ChenAdaptiveRouter（mode: 'browser' | 'hash' | 'auto'，auto在file:协议下自动走 hash）。P1
createStoretearing — 旧实现useReducer+stateRef(render 阶段写) +useEffect通知订阅者，并发模式 / StrictMode 下 state 写入与通知时机错位可能产生 tearing。改为每个StoreProvider实例挂一份makeExternalStore，state 完全脱离 React render/commit 周期。useStoreSelector/createSimpleStore.useStore(selector)/useGlobalStore(store, selector)三个路径之前每次调用selector都返回新引用，触发 React 18 的 "The result of getSnapshot should be cached" 警告、并可能在某些 selector 下死循环。用lastRef缓存{state, selected}+equalityFn参数（默认Object.is），相等时返回上次的引用。useGlobalStore与createSimpleStore.useStore新增可选selector/equalityFn，允许切片订阅。LogosworkspaceStore在 Monaco 打字时能显著减少重渲染。Test plan
npx tsc --noEmit干净npx vitest run62/62 通过（覆盖 createStore / createSimpleStore / useGlobalStore 既有用例）npm run build成功产出dist/ChenAdaptiveRouter打包测试（在 Logos PR 中验）renderToStream末尾</body></html>真的被写出（后续集成测试）Follow-ups (P2, 下一轮 PR)
StaticHandlerContext.status/<Navigate>映射 HTTP 状态码loader/action/ data router 支持handleHotUpdate别对所有pages/**改动 full-reload，尽量保留路由边界的 HMRstartsWith(pagesDir)规范化--install自动装依赖🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests