feat(web-viewer): 引入 vitest 測試骨架 + 修 8 個健壯性風險 (harden-web-viewer-test-resilience) - #146
Conversation
引入 vitest 純函式單測骨架(devDep) + 修 8 個 web-viewer 健壯性風險 (#17 #27 #8 #28 #15 #16 #18 #32);spec delta 於 session-first-review-viewer MODIFIED 3 requirements,新增 spectator-binding / coordinator-boundary / poll-bounded / stream-teardown / sdk-load-failure / spectator-trusts-primary scenarios。baseline:verify 10 passed + test:session-first passed。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- #17 vitest + jsdom(devDep) + 抽 windowHelpers/pollHelpers/envHelpers 純函式 + colocated test(21 passed) - #27 _pollForKitReady 存 _pollForKitReadyId + componentWillUnmount clear(防 unmounted setState) - #8 AppStream 兩處私有 _stream hack 換公開 AppStreamer.terminate(false) - #28 pollForSessionReady MAX_POLL_RETRIES=36 上限 + 逾時 reset + catch reschedule(不再靜默死) - #15 selectSpectatorBinding 用 viewport_sharing.primary_kit_instance_id 顯式挑選(取代 port 隱性) - #16 spectator 分支 stageLoadStatus: 'matched'(信任 primary serving stage) - #18 env.ts 移除 VITE_BIM_CONTROL_API_BASE override(resolveBimControlBase 恆等 coordinator) - #32 index.html 刪無條件 GFN script → AppStream source==='gfn' 動態載入 + onerror 回饋 verify-session-first 既有斷言因 #17 抽函式改查 Window.tsx+windowHelpers 聯集。 驗證:build + vitest 21 + struct-log 10 + session-first + root pytest 65 全綠。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 28 minutes and 34 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR hardens the Web Viewer Test Resilience Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 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 |
There was a problem hiding this comment.
Pull request overview
This PR strengthens web-viewer-sample by introducing a Vitest-based unit test skeleton (pure-function tests) and hardening several demo-time failure modes (bounded polling, safer teardown, spectator readiness fixes, coordinator-boundary enforcement, and conditional GFN SDK loading). It also adds an OpenSpec change artifact documenting the updated viewer requirements and scenarios.
Changes:
- Add Vitest + JSDOM test harness and colocated unit tests for extracted pure helpers.
- Harden viewer/runtime behaviors: bounded polling, timer cleanup on unmount, safer spectator binding/ready-state handling, and public SDK teardown API usage.
- Remove unconditional GFN CDN injection and load the SDK dynamically only for
source === 'gfn', plus tighten env boundary rules.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| web-viewer-sample/vitest.config.ts | Adds Vitest configuration (jsdom, globals, test include glob). |
| web-viewer-sample/tsconfig.json | Adds Vitest globals typing for tests. |
| web-viewer-sample/src/Window.tsx | Imports extracted helpers; clears Kit-ready poll timer on unmount; improves spectator binding selection + spectator stage status. |
| web-viewer-sample/src/utils/windowHelpers.ts | New pure helper module for lifecycle/endpoint comparison + spectator binding selection. |
| web-viewer-sample/src/utils/windowHelpers.test.ts | Unit tests for lifecycle blocking, transport equality, spectator binding selection. |
| web-viewer-sample/src/utils/pollHelpers.ts | New pure helper for bounded poll retry decision. |
| web-viewer-sample/src/utils/pollHelpers.test.ts | Unit tests for bounded poll retry helper. |
| web-viewer-sample/src/config/envHelpers.ts | New helper to enforce coordinator boundary for bim-control base resolution. |
| web-viewer-sample/src/config/envHelpers.test.ts | Unit tests for env base resolution behavior. |
| web-viewer-sample/src/config/env.ts | Removes bim-control env override; makes bimControlApiBase resolve from coordinator base only. |
| web-viewer-sample/src/AppStream.tsx | Uses AppStreamer.terminate(false); dynamically loads GFN SDK only for gfn source. |
| web-viewer-sample/src/App.tsx | Bounds session readiness polling with retry count + max retries and timeout UI/reset behavior. |
| web-viewer-sample/scripts/verify-session-first-contract.mjs | Updates source-level contract checks to include extracted helpers + new resilience assertions. |
| web-viewer-sample/package.json | Adds vitest/jsdom devDeps; adds test/test:watch; includes tests in verify. |
| web-viewer-sample/index.html | Removes unconditional GFN SDK script tag. |
| openspec/changes/harden-web-viewer-test-resilience/tasks.md | Adds implementation task checklist for the change. |
| openspec/changes/harden-web-viewer-test-resilience/proposal.md | Adds rationale and scope for the change. |
| openspec/changes/harden-web-viewer-test-resilience/design.md | Documents design decisions/trade-offs/verification plan. |
| openspec/changes/harden-web-viewer-test-resilience/specs/session-first-review-viewer/spec.md | Spec delta: adds/updates requirements and scenarios around boundary, polling, teardown, spectator behavior, SDK load failure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else { | ||
| console.error(`Session ${sessionId} did not become ready after ${MAX_POLL_RETRIES} retries.`); | ||
| this.setState({ connectionText: "等待 streaming session 就緒逾時，請重試。" }) | ||
| this._resetState() | ||
| } |
| else { | ||
| this.setState({ connectionText: "等待 streaming session 就緒逾時，請重試。" }) | ||
| this._resetState() | ||
| } |
| if (StreamConfig.source === 'gfn') { | ||
| streamSource = StreamType.GFN; | ||
| streamConfig = { | ||
| //@ts-ignore | ||
| GFN : GFN, | ||
| catalogClientId : StreamConfig.gfn.catalogClientId, | ||
| clientId : StreamConfig.gfn.clientId, | ||
| cmsId : StreamConfig.gfn.cmsId, | ||
| onUpdate : (message: StreamEvent) => this._onUpdate(message), | ||
| onStart : (message: StreamEvent) => this._onStart(message), | ||
| onCustomEvent : (message: any) => this._onCustomEvent(message) | ||
| } | ||
| const existing = document.getElementById('gfn-client-sdk-script'); | ||
| if (existing) { | ||
| this._initStream(); | ||
| } else { |
| if (StreamConfig.source === 'gfn') { | ||
| streamSource = StreamType.GFN; | ||
| streamConfig = { | ||
| //@ts-ignore | ||
| GFN : GFN, | ||
| catalogClientId : StreamConfig.gfn.catalogClientId, | ||
| clientId : StreamConfig.gfn.clientId, | ||
| cmsId : StreamConfig.gfn.cmsId, | ||
| onUpdate : (message: StreamEvent) => this._onUpdate(message), | ||
| onStart : (message: StreamEvent) => this._onStart(message), | ||
| onCustomEvent : (message: any) => this._onCustomEvent(message) | ||
| } |
| if (primaryKitInstanceId) { | ||
| return bindings.find((binding) => binding.kit_instance_id !== primaryKitInstanceId) || null; | ||
| } | ||
| return bindings.find((binding) => !sameStreamTransportEndpoint(binding.stream_config, primaryWebrtc)) || null; | ||
| } |
| import { defineConfig } from 'vitest/config' | ||
|
|
||
| export default defineConfig({ | ||
| test: { | ||
| environment: 'jsdom', | ||
| globals: true, | ||
| include: ['src/**/*.{test,spec}.{ts,tsx}'], | ||
| }, | ||
| }) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3ffab2ca1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const script = document.createElement('script'); | ||
| script.id = 'gfn-client-sdk-script'; | ||
| script.src = 'https://sdk.nvidia.com/gfn/client-sdk/1.x/gfn-client-sdk.js'; | ||
| script.onload = () => this._initStream(); |
There was a problem hiding this comment.
When StreamConfig.source === 'gfn' and this component unmounts before the CDN script finishes loading, this onload handler still calls _initStream() on the stale instance. Because componentWillUnmount() has already terminated the streamer, the late handler can start a hidden AppStreamer connection that races the current viewer/reconnect flow; clear or guard the handler on unmount before connecting.
Useful? React with 👍 / 👎.
| static stop() { | ||
| AppStreamer.stop(); | ||
| (AppStreamer as any)._stream = null; // Accessing a private member | ||
| AppStreamer.terminate(false); | ||
| } |
There was a problem hiding this comment.
In the reconnect path I inspected (Window.tsx calls AppStream.stop() and immediately increments streamMountKey), this new stop() implementation starts AppStreamer.terminate(false) but does not return or await its promise. The SDK only clears its singleton stream after that promise resolves, so a remounted AppStream can call connect() while termination is still in progress and get stuck in the same busy/disconnecting state the reconnect button is meant to recover from.
Useful? React with 👍 / 👎.
| const existing = document.getElementById('gfn-client-sdk-script'); | ||
| if (existing) { | ||
| this._initStream(); |
There was a problem hiding this comment.
If a GFN reconnect/remount happens after the first mount appended gfn-client-sdk-script but before that script has loaded, this branch sees the element and immediately calls _initStream(). At that point the GFN global is still undefined, so _initStream() throws instead of waiting for the pending script's load/error event; track the loaded state or attach another listener before initializing.
Useful? React with 👍 / 👎.
| showUI: true, | ||
| isLoading: false, | ||
| loadingText: "旁觀串流已連線", | ||
| stageLoadStatus: 'matched', |
There was a problem hiding this comment.
For any URL opened with streamRole=spectator/view_only, a successful WebRTC start now marks stageLoadStatus as matched without checking latestStreamConfig.viewport_sharing?.spectator_ready or any stage-load evidence. When the coordinator has not marked the spectator Kit ready (or spectator mode fell back to the primary/only binding), this makes Runtime ready report yes even though the primary stage may not be serving the expected artifact yet.
Useful? React with 👍 / 👎.
PR Review Agent Summary
Blockers
Warnings
Validation Commands
Checks
Human Review Notes
|
- #27(P2): _pollForKitReady 進入點改 _clearPollForKitReady(),閉合 in-mount 重入孤兒並行 chain(原僅 id=null 不 clearTimeout) - #28(P2): _resetState 參數化 connectionText,逾時訊息不再被 _resetState 的 connectionText:'' 覆寫 - #32(P2): GFN script 命中既有但全域 GFN 未就緒時補掛 load/error,避免 remount-during-load 仍 ReferenceError(用 //@ts-ignore 不引入 as any) - design.md #8: 修正捏造的 SDK 引用(實裝 5.17.0/L71/terminate(terminateApp?) 無 _force) - 補 triReady.test.ts(11 test,含 #16 spectator started+matched→yes);Decision 7 誠實揭露 structLog.test.ts defer 驗證:build + vitest 32 passed(21→32) + struct-log 10 + session-first 全綠。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR Review Agent Summary
Blockers
Warnings
Validation Commands
Checks
Human Review Notes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
web-viewer-sample/src/App.tsx (1)
149-152: 💤 Low valueUntracked
setTimeoutpoll chain has no cleanup.
pollForSessionReadyschedules retries withsetTimeoutbut never stores the handle, so a pending poll can fire after teardown and callsetState.Appis the root component (low unmount likelihood), but this mirrors the timer race fixed for_pollForKitReady(#27) inWindow.tsx; consider tracking and clearing the handle for parity and to guard against future remounts.Also applies to: 159-161
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-viewer-sample/src/App.tsx` around lines 149 - 152, The retry setTimeout in pollForSessionReady (and its twin at lines ~159-161) is untracked and can fire after unmount causing setState; modify pollForSessionReady to store the timeout handle (e.g., this.sessionPollTimeout or an array this.pollTimeouts) when calling setTimeout, and add cleanup logic that clears those handles in componentWillUnmount (or useEffect cleanup) so pending timers are cleared; mirror the approach used for _pollForKitReady to locate where to add the handle storage and clear calls.web-viewer-sample/src/Window.tsx (1)
982-992: 💤 Low valueConsider adding a retry limit to prevent unbounded polling.
The
_pollForKitReadymethod polls indefinitely until Kit responds or the component unmounts. While timer cleanup on unmount (line 312) prevents memory leaks, if Kit never responds due to a permanent failure, the viewer will continue polling until user action.Consider adding a retry counter and fallback similar to the MAX_POLL_RETRIES pattern used elsewhere in the codebase to surface a user-visible error after a reasonable timeout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-viewer-sample/src/Window.tsx` around lines 982 - 992, Add a retry limit to the indefinite poll in _pollForKitReady: introduce a MAX_POLL_RETRIES constant and a counter field (e.g., _pollForKitReadyAttempts) incremented each time _pollForKitReady runs; if attempts exceed MAX_POLL_RETRIES call _clearPollForKitReady(), stop scheduling further timeouts, and set a user-visible fallback state (e.g., setState({ isKitReady: false, kitError: true }) or call an existing error handler). Reset the attempts counter when polling starts successfully (when isKitReady becomes true) and clear it in componentWillUnmount or inside _clearPollForKitReady so cleanup is consistent; update any places that start polling (e.g., _onStreamStarted) to reset the counter before calling _pollForKitReady.web-viewer-sample/src/AppStream.tsx (1)
73-87: ⚖️ Poor tradeoffPotential missed error on script remount.
If the GFN SDK script has already failed to load before
componentDidMountruns (e.g., on component remount after a prior failure), the script element exists butGFNis undefined, and the error event won't re-fire. The added error listener at line 82-85 won't catch the already-completed failure.Consider checking the script element's error state or the global
GFNpresence with a timeout fallback to detect stale failures:const existing = document.getElementById('gfn-client-sdk-script'); if (existing) { if (typeof GFN !== 'undefined') { this._initStream(); } else { const timeout = setTimeout(() => { console.error('GFN SDK did not load within expected time'); this.props.onStreamFailed(); }, 5000); existing.addEventListener('load', () => { clearTimeout(timeout); this._initStream(); }); existing.addEventListener('error', () => { clearTimeout(timeout); console.error('Failed to load GFN client SDK script'); this.props.onStreamFailed(); }); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-viewer-sample/src/AppStream.tsx` around lines 73 - 87, When the script element 'gfn-client-sdk-script' exists but GFN is undefined, add a timeout fallback and/or check the script element's ready state so a prior failed load is detected: if typeof GFN === 'undefined' and existing.readyState indicates completed/error or a timeout (e.g., 5s) elapses, call this.props.onStreamFailed(), otherwise attach load and error listeners that clear the timeout and either call this._initStream() on load or call this.props.onStreamFailed() on error; ensure the timeout is cleared in both listeners to avoid stray failure callbacks.web-viewer-sample/src/utils/windowHelpers.ts (1)
77-86: 💤 Low valueEdge case: Multiple bindings with same kit_instance_id but different transports.
If
primaryKitInstanceIdis provided but all bindings share the samekit_instance_idvalue (with different transports), this function returnsnull, and the caller falls back toprimaryBinding. This means spectator and primary would connect to the same endpoint.However, this scenario would indicate a coordinator configuration error, since properly deployed Kit instances should have unique IDs when
primary_kit_instance_idis provided. The current implementation correctly prioritizes explicit ID-based selection over transport inference.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-viewer-sample/src/utils/windowHelpers.ts` around lines 77 - 86, The selectSpectatorBinding function currently returns null when primaryKitInstanceId is provided but no binding has a different kit_instance_id, causing the caller to reuse the primary endpoint; change selectSpectatorBinding to first try id-based selection (bindings.find(binding => binding.kit_instance_id !== primaryKitInstanceId)) and if that returns null, fall back to transport-based selection by returning bindings.find(binding => !sameStreamTransportEndpoint(binding.stream_config, primaryWebrtc)) || null; update the function body (selectSpectatorBinding and its use of sameStreamTransportEndpoint and primaryWebrtc) so a spectator with a different transport is chosen even when kit_instance_ids are all the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web-viewer-sample/src/config/envHelpers.test.ts`:
- Around line 3-11: Add a test to cover the null case for resolveBimControlBase:
when queryCoordinatorApiBase is null the function should fall back to the
envCoordinatorApiBase; update the describe block to include an it test that
calls resolveBimControlBase(null, "e") and asserts the result equals "e" so the
string|null path is exercised (reference resolveBimControlBase in
envHelpers.test.ts).
---
Nitpick comments:
In `@web-viewer-sample/src/App.tsx`:
- Around line 149-152: The retry setTimeout in pollForSessionReady (and its twin
at lines ~159-161) is untracked and can fire after unmount causing setState;
modify pollForSessionReady to store the timeout handle (e.g.,
this.sessionPollTimeout or an array this.pollTimeouts) when calling setTimeout,
and add cleanup logic that clears those handles in componentWillUnmount (or
useEffect cleanup) so pending timers are cleared; mirror the approach used for
_pollForKitReady to locate where to add the handle storage and clear calls.
In `@web-viewer-sample/src/AppStream.tsx`:
- Around line 73-87: When the script element 'gfn-client-sdk-script' exists but
GFN is undefined, add a timeout fallback and/or check the script element's ready
state so a prior failed load is detected: if typeof GFN === 'undefined' and
existing.readyState indicates completed/error or a timeout (e.g., 5s) elapses,
call this.props.onStreamFailed(), otherwise attach load and error listeners that
clear the timeout and either call this._initStream() on load or call
this.props.onStreamFailed() on error; ensure the timeout is cleared in both
listeners to avoid stray failure callbacks.
In `@web-viewer-sample/src/utils/windowHelpers.ts`:
- Around line 77-86: The selectSpectatorBinding function currently returns null
when primaryKitInstanceId is provided but no binding has a different
kit_instance_id, causing the caller to reuse the primary endpoint; change
selectSpectatorBinding to first try id-based selection (bindings.find(binding =>
binding.kit_instance_id !== primaryKitInstanceId)) and if that returns null,
fall back to transport-based selection by returning bindings.find(binding =>
!sameStreamTransportEndpoint(binding.stream_config, primaryWebrtc)) || null;
update the function body (selectSpectatorBinding and its use of
sameStreamTransportEndpoint and primaryWebrtc) so a spectator with a different
transport is chosen even when kit_instance_ids are all the same.
In `@web-viewer-sample/src/Window.tsx`:
- Around line 982-992: Add a retry limit to the indefinite poll in
_pollForKitReady: introduce a MAX_POLL_RETRIES constant and a counter field
(e.g., _pollForKitReadyAttempts) incremented each time _pollForKitReady runs; if
attempts exceed MAX_POLL_RETRIES call _clearPollForKitReady(), stop scheduling
further timeouts, and set a user-visible fallback state (e.g., setState({
isKitReady: false, kitError: true }) or call an existing error handler). Reset
the attempts counter when polling starts successfully (when isKitReady becomes
true) and clear it in componentWillUnmount or inside _clearPollForKitReady so
cleanup is consistent; update any places that start polling (e.g.,
_onStreamStarted) to reset the counter before calling _pollForKitReady.
🪄 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: 0e9da65c-8099-4df9-945a-947b4c68bc79
📒 Files selected for processing (20)
openspec/changes/harden-web-viewer-test-resilience/design.mdopenspec/changes/harden-web-viewer-test-resilience/proposal.mdopenspec/changes/harden-web-viewer-test-resilience/specs/session-first-review-viewer/spec.mdopenspec/changes/harden-web-viewer-test-resilience/tasks.mdweb-viewer-sample/index.htmlweb-viewer-sample/package.jsonweb-viewer-sample/scripts/verify-session-first-contract.mjsweb-viewer-sample/src/App.tsxweb-viewer-sample/src/AppStream.tsxweb-viewer-sample/src/Window.tsxweb-viewer-sample/src/config/env.tsweb-viewer-sample/src/config/envHelpers.test.tsweb-viewer-sample/src/config/envHelpers.tsweb-viewer-sample/src/utils/pollHelpers.test.tsweb-viewer-sample/src/utils/pollHelpers.tsweb-viewer-sample/src/utils/triReady.test.tsweb-viewer-sample/src/utils/windowHelpers.test.tsweb-viewer-sample/src/utils/windowHelpers.tsweb-viewer-sample/tsconfig.jsonweb-viewer-sample/vitest.config.ts
💤 Files with no reviewable changes (1)
- web-viewer-sample/index.html
| else { | ||
| console.error(`Session ${sessionId} did not become ready after ${MAX_POLL_RETRIES} retries.`); | ||
| this._resetState("等待 streaming session 就緒逾時，請重試。") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for connectionText rendering across the app and the forms
rg -nP --type=tsx -C3 'connectionText'
fd -e tsx -i 'forms' --exec rg -nP -C3 'connectionText' {}Repository: monkey1sai/AI-BIM-governance
Length of output: 100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find relevant code locations in App.tsx
rg -n "connectionText" web-viewer-sample/src/App.tsx
rg -n "_resetState" web-viewer-sample/src/App.tsx
rg -n "Forms\.IDLE|currentForm|enum Forms|AppOnly|Stream" web-viewer-sample/src/App.tsx
# Show specific nearby regions for the lines referenced in the review comment
sed -n '80,140p' web-viewer-sample/src/App.tsx
sed -n '160,220p' web-viewer-sample/src/App.tsx
sed -n '260,340p' web-viewer-sample/src/App.tsxRepository: monkey1sai/AI-BIM-governance
Length of output: 15667
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Ensure file exists
ls -la web-viewer-sample/src/App.tsx
# Find relevant code locations in App.tsx
rg -n "connectionText" web-viewer-sample/src/App.tsx
rg -n "_resetState" web-viewer-sample/src/App.tsx
rg -n "Forms\.IDLE|currentForm|AppOnly|Stream" web-viewer-sample/src/App.tsx
# Show specific nearby regions for the lines referenced in the review comment
sed -n '90,140p' web-viewer-sample/src/App.tsx
sed -n '150,210p' web-viewer-sample/src/App.tsx
sed -n '270,320p' web-viewer-sample/src/App.tsxRepository: monkey1sai/AI-BIM-governance
Length of output: 14074
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la web-viewer-sample/src/App.tsx
rg -n "connectionText" web-viewer-sample/src/App.tsx
rg -n "_resetState" web-viewer-sample/src/App.tsx
rg -n "Forms\.IDLE|currentForm|AppOnly|Stream" web-viewer-sample/src/App.tsx
sed -n '90,140p' web-viewer-sample/src/App.tsx
sed -n '150,210p' web-viewer-sample/src/App.tsx
sed -n '270,320p' web-viewer-sample/src/App.tsxRepository: monkey1sai/AI-BIM-governance
Length of output: 14074
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la web-viewer-sample/src/App.tsx
rg -n "connectionText" web-viewer-sample/src/App.tsx
rg -n "_resetState" web-viewer-sample/src/App.tsx
rg -n "Forms\.IDLE|currentForm|AppOnly|Stream" web-viewer-sample/src/App.tsx
sed -n '90,140p' web-viewer-sample/src/App.tsx
sed -n '150,210p' web-viewer-sample/src/App.tsx
sed -n '270,320p' web-viewer-sample/src/App.tsxRepository: monkey1sai/AI-BIM-governance
Length of output: 14074
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la web-viewer-sample/src/App.tsx
rg -n "connectionText" web-viewer-sample/src/App.tsx
rg -n "_resetState" web-viewer-sample/src/App.tsx
rg -n "Forms\.IDLE|currentForm|AppOnly|Stream" web-viewer-sample/src/App.tsx
sed -n '90,140p' web-viewer-sample/src/App.tsx
sed -n '150,210p' web-viewer-sample/src/App.tsx
sed -n '270,320p' web-viewer-sample/src/App.tsxRepository: monkey1sai/AI-BIM-governance
Length of output: 14074
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "connectionText" web-viewer-sample/srcRepository: monkey1sai/AI-BIM-governance
Length of output: 738
Fix hidden timeout feedback in pollForSessionReady (App.tsx).
pollForSessionReady calls this._resetState("等待 streaming session 就緒逾時，請重試。") on timeout, but _resetState(...) switches currentForm to Forms.Stream (local) / Forms.AppOnly (non-local). connectionText is only rendered when currentForm === Forms.IDLE, so the timeout message won’t be visible to the user. Also, the retry loop uses setTimeout without canceling when _resetState runs, so it can keep updating state after the UI has moved on.
- #32(A): _initStream 改 globalThis.GFN 讀取,缺失走 onStreamFailed,避免 onload 後 GFN 未建立仍裸變數 ReferenceError(Copilot+Codex) - #15(B): selectSpectatorBinding 先驗 primaryKitInstanceId 在 bindings 內才用 id 挑,否則退 port-diff,避免 coordinator 資料不一致時誤選 primary(Copilot) - #16(D): spectator 僅在 viewport_sharing.spectator_ready 時 stageLoadStatus='matched',否則 pending(Codex);連帶 verify 斷言 + spec scenario 精確化 - envHelpers.test 補 null case(CodeRabbit);vitest.config 補分號對齊 .prettierrc semi(Copilot) 驗證:build + vitest 34 passed(32→34) + struct-log 10 + session-first 全綠。 defer(PR 揭露):reconnect await terminate、GFN unmount guard(pre-existing async / gfn 窄窗競態)。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review 回應（opus 對抗式 + Copilot / Codex / CodeRabbit）兩輪 review fix 已 push（ 已修（真問題 / proposal 目標缺口）
Defer（pre-existing / 跨輪追蹤，本 PR 不擴大 scope）
驗證：web-viewer |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3dfd04151
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| else { | ||
| console.error(`Session ${sessionId} did not become ready after ${MAX_POLL_RETRIES} retries.`); | ||
| this._resetState("等待 streaming session 就緒逾時，請重試。") |
There was a problem hiding this comment.
When a newly created stream returns 202 and stays non-ready through MAX_POLL_RETRIES, this path resets the UI state and clears sessionId without calling destroyStreamingSession for the already-created session. If the backend finishes provisioning after this timeout, the viewer has hidden the End Stream control and lost the id needed to release the Kit/GPU session, so the resource can keep running until an operator cleans it up.
Useful? React with 👍 / 👎.
| loadingText: spectatorReady ? "旁觀串流已連線" : "旁觀串流已連線，等待 primary stage 就緒", | ||
| stageLoadStatus: spectatorReady ? 'matched' : 'pending', |
There was a problem hiding this comment.
When a spectator connects before viewport_sharing.spectator_ready is true, this branch leaves stageLoadStatus at pending and returns without scheduling any stream-config refresh. I checked the Socket.IO onEvent path in this file; it only appends/logs events, so if the coordinator later flips spectator_ready after the primary stage loads, this viewer keeps reporting Runtime ready as incomplete until a full reload/manual bootstrap even though the coordinator is now ready.
Useful? React with 👍 / 👎.
| coordinatorApiBase: queryCoordinatorApiBase || envCoordinatorApiBase, | ||
| coordinatorSocketUrl: queryCoordinatorSocketUrl || queryCoordinatorApiBase || import.meta.env.VITE_COORDINATOR_SOCKET_URL || envCoordinatorApiBase, | ||
| bimControlApiBase: import.meta.env.VITE_BIM_CONTROL_API_BASE || queryCoordinatorApiBase || envCoordinatorApiBase, | ||
| bimControlApiBase: resolveBimControlBase(queryCoordinatorApiBase, envCoordinatorApiBase), |
There was a problem hiding this comment.
With this base now forced to the coordinator, BimControlClient.getReviewSessionRequest() and .getArtifacts() call /api/review-session-requests/... and /api/model-versions/.../artifacts on the coordinator whenever the viewer is opened with reviewRequestId or falls back to metadata loading. I searched the coordinator routes and found only /api/review-sessions and /api/external/ifc-ready families, not these BimControlClient paths, so those launches now 404 before they can create/bind the review session unless equivalent coordinator endpoints are added or the call sites are removed.
Useful? React with 👍 / 👎.
PR Review Agent Summary
Blockers
Warnings
Validation Commands
Checks
Human Review Notes
|
回應 Codex 第二輪 P2（App.tsx L155 / Window.tsx L958 / env.ts L77）三項已查證，均為 enhancement / 既有 gap，非本 PR 引入的 regression，列為 follow-up（已寫入
兩輪已修所有真 bug / proposal 目標缺口；CI 全綠、mergeable CLEAN。 |
PR Review Agent Summary
Blockers
Warnings
Validation Commands
Checks
Human Review Notes
|
CH-4(web-viewer vitest 骨架 + 8 健壯性風險)archive:session-first-review-viewer MODIFIED 3 requirements 合入 specs;roadmap §1.6 加對齊紀錄(雙層多輪 review + defer follow-up 揭露)。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
為什麼
web-viewer-sample目前零 test framework（只有 4 個自製.mjssource-level smoke），且散落多個「demo 當下才爆」的脆弱點:無上限 poll、spectator 永遠 incomplete、CSP 下 GFNReferenceError炸整頁、env 繞過 coordinator boundary、私有 SDK hack、unmount 後 timer 仍 setState。本 PR 引入 vitest 純函式單測骨架（devDep）並修掉 8 個健壯性風險,把「demo 當下才爆」變「build 時就有 test / 防禦擋住」。改了什麼
vitest（^1.6）+jsdom（^24）到 devDependencies;新增獨立vitest.config.ts;verify鏈加test;抽windowHelpers/pollHelpers/envHelpers純函式 + colocated*.test.ts（21 passed）。_pollForKitReady存_pollForKitReadyId+componentWillUnmountclear（防 unmounted setState 與並行 poll chain）。AppStream兩處私有(AppStreamer as any)._stream = nullhack 換成公開AppStreamer.terminate(false)（已從實裝 SDK 5.6.0.d.ts驗證為公開 API,不殺 server 端 Kit）。pollForSessionReady加MAX_POLL_RETRIES = 36上限 + 逾時_resetState+ 錯誤回饋;catch 分支改 reschedule 到上限（不再靜默殺死整條 poll）。selectSpectatorBinding改用既有viewport_sharing.primary_kit_instance_id顯式辨識 primary（取代 port 隱性排除式挑選）,primary id 缺失退回 port-diff fallback。零跨 repo、零新欄位。stageLoadStatus: 'matched'（信任 coordinator 保證spectator_ready的 primary serving stage）,使 spectator Runtime ready 能轉yes。src/config/env.ts移除VITE_BIM_CONTROL_API_BASEoverride,bimControlApiBase恆等於coordinatorApiBase（前端不得經 env 繞過 coordinator boundary）。index.html無條件 GFN CDN script;改AppStream僅在source === 'gfn'時動態載入 +onerror失敗回饋（CSP 阻擋 / 離線不炸整頁）。範圍邊界
web-viewer-sample;不改bim-review-coordinator/bim-streaming-server/ 任何對外 contract（coordinator REST / Socket.IO / DataChannel JSON / callback payload）。devDependencies（vitest / jsdom）,不新增 production dependency;不升級@nvidia/omniverse-webrtc-streaming-library（terminate(false)在已釘的 ^5.6.0 即可用）。BimControlClient → ReviewMetadataClient純改名 defer 為 optional follow-up。驗證
web-viewer-sample:npm run verify（vite build + vitest 21 passed + struct-log 10 passed） ✓npm run test:session-first（含 5 個新 source-level 契約斷言 docs(workflow): 補齊 roadmap HTML 檢視版流程 #27/docs: v3 依新版架構圖 v1+v2 重寫專案開發流程（7 層架構 + IFC→USD 品質管線） #8/docs(openspec): 延後多 Kit 驗證至 GPU 部署後 #28/docs(verification): 2026-05-08 review-session spec end-to-end verification #16/feat(coordinator): 建立會議生命週期稽核事件 #32） ✓.venv\Scripts\python.exe -m pytest tests→ 65 passed ✓pollForSessionReady/_resolveStreamEndpoint/_pollForKitReady全 LOW（d=1 caller 皆同檔,不改簽章）。OpenSpec
harden-web-viewer-test-resiliencesession-first-review-viewerMODIFIED 3 requirements,新增 spectator-binding / coordinator-boundary / poll-bounded / stream-teardown / sdk-load-failure / spectator-trusts-primary scenarios。已知風險 / 後續
.mjs暫與 vitest 並存;verify-session-first因 feat(worker): 保留原始 IFC 檔名追蹤 #17 抽函式改查Window.tsx + windowHelpers.ts聯集,長期保留為跨檔 contract guard。BimControlClient → ReviewMetadataClient改名 defer。Summary by CodeRabbit
New Features
Bug Fixes
Tests