Repository navigation
feat(console): 合併模型資料並修正 A1 MinIO 檢核 - #303
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7ESqHTEqCYE8PnzRHmRZc
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7ESqHTEqCYE8PnzRHmRZc
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7ESqHTEqCYE8PnzRHmRZc
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7ESqHTEqCYE8PnzRHmRZc
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7ESqHTEqCYE8PnzRHmRZc
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7ESqHTEqCYE8PnzRHmRZc
…（MD 合一 Task 7） Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7ESqHTEqCYE8PnzRHmRZc
…sk 8） Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W7ESqHTEqCYE8PnzRHmRZc
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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 consolidates the previously separate Conversion (#conv), MinIO (#minio), and Intake (#intake) console views into a single Model Data page rooted at #minio / ModelDataPage, with in-shell alias redirects preserving the old hashes and their query strings. It also fixes the A1 governance workflow so that, when a review session is selected, rule-runs go through the coordinator's rule-runs/for-session proxy (server-local IFC path resolved coordinator-side), and the "Open 3D Review Room" handoff is no longer gated by session selection.
Changes:
- Merge Model Data / Conversion / Intake into
ModelDataPage, extracting folder/conversion state into new hooks (useMinioFolder,useConversionData,useConversionActions) and presentational panes (MinioTreePane,GlobalConversionPane,ObjectDetailPane), plusEdgeConsolealias redirects. - A1
doRunnow routes tocreateRuleRunForSessionwhen a session is selected, and the Review Room handoff (buildA1ReviewRoomHandoffHash/a1ReviewRoomHandoffReason) makessessionIdoptional. - Adds
createRoot/act-based unit tests for the new hooks, panes, shell, and alias redirect behavior.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
web-viewer-sample/src/console/pages.tsx |
A1 for-session rule-run routing, optional-session Review Room handoff, updated captions/lead copy; missing sessions dep in doRun. |
web-viewer-sample/src/console/modelData/useMinioFolder.ts |
New hook: folder fetch/cache, generation guards, SSE dirty-signal invalidation. |
web-viewer-sample/src/console/modelData/useConversionData.ts |
New hook: loads ifc-ready jobs, watch status, ledger records, conversion history. |
web-viewer-sample/src/console/modelData/useConversionActions.ts |
New hook: prioritize/retry/watch-toggle/trigger mutations with ref guards. |
web-viewer-sample/src/console/modelData/GlobalConversionPane.tsx |
New pane; sets data-highlight on matched rows (no CSS rule exists yet). |
web-viewer-sample/src/console/modelData/MinioTreePane.tsx |
New tree pane consuming useMinioFolder. |
web-viewer-sample/src/console/modelData/ObjectDetailPane.tsx |
New single-object detail pane. |
web-viewer-sample/src/console/modelData/ModelDataPage.tsx |
New shell composing hooks + panes and handling incoming handoffs. |
web-viewer-sample/src/console/EdgeConsole.tsx |
Alias redirect (#conv/#intake → #minio), route/axis wiring. |
web-viewer-sample/src/console/data.ts |
Nav items collapsed into the 模型資料與轉檔 entry. |
web-viewer-sample/src/console/edge-console.css |
Adds .md-split* layout and an un-scoped [data-selected="true"] rule. |
web-viewer-sample/src/console/*.test.tsx / *.test.ts |
New/updated tests for hooks, panes, shell, and alias redirect. |
Comments suppressed due to low confidence (1)
web-viewer-sample/src/console/pages.tsx:418
doRunstill readssessionsat line 400 (sessions.find((s) => s.session_id === selectedSession)?.expected_mapping_url), butsessionswas removed from thisuseCallbackdependency array. Since the repo extendsplugin:react-hooks/recommended, this triggers areact-hooks/exhaustive-depswarning, and the callback captures a stalesessionssnapshot. In practicesessionsis fetched once on mount so it usually stays in sync, but the omission is a latent stale-closure risk. Restoringsessionsto the dependency array keeps the callback correct and lint-clean.
coordinatorClient.getMinioObjects()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return ( | ||
| <Fragment key={j.ifc_ready_job_id}> | ||
| {/* (d) handoff job_id 命中列高亮（ifc_ready_job_id 或 conversion_job_id 命中） */} | ||
| <tr data-highlight={highlightJobId != null && (j.ifc_ready_job_id === highlightJobId || j.conversion_job_id === highlightJobId)}> |
| @media (max-width: 1100px) { .md-split { grid-template-columns: 1fr; } } | ||
| .md-split-tree { min-width: 0; } | ||
| .md-split-main { min-width: 0; } | ||
| [data-selected="true"] { outline: 1px solid var(--ec-accent, #7fd962); border-radius: 4px; } |
![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: 35b768aa1f
ℹ️ 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 { rule_run_id } = selectedSession | ||
| ? await governanceClient.createRuleRunForSession(selectedSession, { ids_path: idsPath || undefined }) |
There was a problem hiding this comment.
When a review session is selected, this branch stops sending state.ifcPath/the MinIO key and calls the for-session endpoint with only the session id and IDS path. In an environment with multiple active sessions, an operator can pick model A in the MinIO selector but choose session B from the dropdown; the rule run will then be created for session B's server-local IFC while the UI still presents model A as the selected model and uses that context for follow-up links. Please either prove/bind the selected session to the picked MinIO object before using this endpoint, or keep the direct selected-file path flow when the session is not known to belong to that file.
Useful? React with 👍 / 👎.
| onClick={() => onLocateObject(objectKey)} | ||
| >{t("檔案 →", "File →")}</Btn> | ||
| ) : ( | ||
| <span className="ec-note">{t("非 MinIO 來源", "non-MinIO source")}</span> |
There was a problem hiding this comment.
When /api/conversion/records is truncated or fails, the records.find(...) miss above does not prove this job is non-MinIO; the matching ledger row may simply be outside the returned window or unavailable. In that case this fallback labels real MinIO jobs as non-MinIO source and suppresses the File/Trigger actions, so large ledgers or a transient records outage block operators from locating or retrying affected conversions. Use the hook's incomplete/error state to render an indeterminate source instead of treating a miss as definitive.
Useful? React with 👍 / 👎.
| if (h.job_id) { | ||
| if (!data.jobsLoaded) return "indeterminate"; | ||
| if (data.jobs.some((j) => j.ifc_ready_job_id === h.job_id || j.conversion_job_id === h.job_id)) return true; | ||
| return data.jobsTruncated ? "indeterminate" : false; |
There was a problem hiding this comment.
If /api/external/ifc-ready fails while opening a #minio?...job_id=... handoff, useConversionData still marks jobsLoaded=true with an empty list and this branch returns false unless the successful response was truncated. That turns a coordinator outage or transient 502 into a red not_found banner for a real job id from A1/intake instead of the honest indeterminate state, so operators are told the job does not exist when the authority was simply unavailable. Please account for jobsErr/an incomplete jobs state before declaring the id absent.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-viewer-sample/src/console/pages.tsx (1)
379-418: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
doRun's dependency array omitssessions, which the callback body reads.Line 400 reads
sessions.find((s) => s.session_id === selectedSession)?.expected_mapping_url, butsessionsisn't in theuseCallbackdeps (line 418). Unlike other intentional dependency omissions in this file (which all carry an explanatory comment/eslint-disable), this one has none — it looks like an oversight from the session-optionality refactor rather than a deliberate choice. Ifsessionsupdates withoutselectedSession(or another listed dep) also changing,doRunwould use a stalesessionssnapshot for mapping enrichment.🔧 Suggested fix
- }, [state.step, state.runError, state.ifcPath, idsPath, selectedSession]); + }, [state.step, state.runError, state.ifcPath, idsPath, selectedSession, sessions]);🤖 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/console/pages.tsx` around lines 379 - 418, The doRun callback reads sessions when deriving expected_mapping_url for mapping enrichment, but sessions is missing from the useCallback dependency array, so the callback can use a stale sessions snapshot. Update the dependency list on doRun to include sessions (alongside the existing state.step, state.runError, state.ifcPath, idsPath, and selectedSession references) so the lookup stays in sync with current session data.
🧹 Nitpick comments (6)
web-viewer-sample/src/console/modelData/useMinioFolder.ts (1)
35-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for the generation race-guard.
This is the most intricate logic in the cohort (concurrent-load protection via
loadGenRef), and unlikeuseConversionData.ts(which shipsuseConversionData.test.tswith explicit isolation/truncation assertions), there's no accompanying test file verifying the race-guard behavior (e.g., a slow root-layer response resolving after a fast navigated-layer response should not overwrite the newer folder). Given the samecreateRoot/act/waitForharness pattern already exists in this cohort, adding a regression test here would be low-effort.🤖 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/console/modelData/useMinioFolder.ts` around lines 35 - 67, Add regression coverage for the `loadGenRef` race-guard in `useMinioFolder`; the current `load` callback’s concurrent-request protection is untested. Create a test alongside `useMinioFolder` using the existing `createRoot`/`act`/`waitFor` pattern to simulate a slow earlier `coordinatorClient.getMinioFolder` resolving after a faster later navigation, and assert the stale response does not overwrite the newer folder state. Reference the `load`, `loadGenRef`, and `coordinatorClient.getMinioFolder` behavior directly in the test setup.web-viewer-sample/src/console/modelData/MinioTreePane.tsx (1)
112-153: 🚀 Performance & Scalability | 🔵 TrivialLGTM overall; minor perf nit on
ledgerChipStatus.
ledgerChipStatus(Line 116) is computed for every object regardless of role, even though the chip is only rendered forrole === "source_ifc"(Line 144). Not worth reworking given typical folder sizes, but could be pushed inside thesource_ifcbranch to avoid wasted lookups onother/parsed_usdcrows.🤖 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/console/modelData/MinioTreePane.tsx` around lines 112 - 153, Move the ledger status lookup in MinioTreePane’s folder object render so `ledgerChipStatus(idk, records, recordsIncomplete)` is only called for `obj.role === "source_ifc"`, since the chip is only rendered there. Keep the existing `ledgerChipStatus`, `MINIO_CHIP_LABEL`, and `minio-chip-${idk}` logic together inside that branch so non-source_ifc rows skip the extra lookup.web-viewer-sample/src/console/modelData/GlobalConversionPane.tsx (1)
39-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
toggleCoverageis duplicated near-verbatim withObjectDetailPane.tsx.Both this file and
ObjectDetailPane.tsximplement the same coverage-toggle open/collapse/cache/error logic (comments even note it was "搬自 CV toggleCoverage 原文" — copy/pasted into two places). Consider extracting a shareduseCoverageDrawer(job)hook (mirroring the pattern ofuseConversionActions) so both panes consume one implementation and future bugfixes/behavior changes don't need to be applied twice.♻️ Sketch of a shared hook
// modelData/useCoverageDrawer.ts export function useCoverageDrawer() { const [openJob, setOpenJob] = useState<string | null>(null); const [cov, setCov] = useState<Record<string, CovState>>({}); const toggle = useCallback(async (job: IfcReadyListItem) => { /* shared body */ }, [openJob, cov]); return { openJob, cov, toggle }; }🤖 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/console/modelData/GlobalConversionPane.tsx` around lines 39 - 58, The `toggleCoverage` logic in `GlobalConversionPane` is duplicated with the matching implementation in `ObjectDetailPane`, so extract the shared open/collapse/cache/error flow into a reusable hook such as `useCoverageDrawer` (similar to `useConversionActions`) and have both panes call that single source of truth. Keep the hook responsible for the `openJob`/`cov` state and the async `toggleCoverage` behavior so future changes only need to be made once.web-viewer-sample/src/console/modelData/ObjectDetailPane.test.tsx (1)
59-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixture factories duplicated with
ModelDataPage.test.tsx.
makeObject/makeRecord/makeJob/makeData(and theKconstant) here are essentially identical to the ones defined inModelDataPage.test.tsx. Combined with thewaitForduplication noted inMinioTreePane.test.tsx, this suggests a sharedmodelData/testFixtures.ts(or similar) would reduce copy/paste drift across the four new test files in this directory.🤖 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/console/modelData/ObjectDetailPane.test.tsx` around lines 59 - 69, The test fixture helpers in ObjectDetailPane.test.tsx duplicate the same model-data factories used in ModelDataPage.test.tsx, so extract the shared `K` constant and the `makeObject`/`makeRecord`/`makeJob`/`makeData` helpers into a common model-data test fixture module and import them from both specs. Keep the existing helper names or re-export them so the other test files in this directory can reuse the same fixtures and avoid copy/paste drift.web-viewer-sample/src/console/modelData/MinioTreePane.test.tsx (1)
26-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
waitForpolling helper is duplicated across four test files.The identical
waitForimplementation (poll +act+ assert-retry) reappears verbatim inGlobalConversionPane.test.tsx,ObjectDetailPane.test.tsx, andModelDataPage.test.tsx. Consider extracting to a shared test-utils module (e.g.,modelData/testUtils.ts) under this newmodelData/directory to avoid drift if the flaky-test workaround needs tuning later.🤖 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/console/modelData/MinioTreePane.test.tsx` around lines 26 - 33, The waitFor polling helper is duplicated across the modelData test files, so extract the shared poll-plus-act retry logic from MinioTreePane.test.tsx into a common test-utils module under modelData and update GlobalConversionPane.test.tsx, ObjectDetailPane.test.tsx, and ModelDataPage.test.tsx to import it. Keep the shared helper behavior identical by preserving the same signature, act-based tick, assert retry loop, and maxTicks default so all tests use one implementation and future tweaks stay consistent.web-viewer-sample/src/console/modelData/ObjectDetailPane.tsx (1)
65-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate coverage-toggle logic with GlobalConversionPane.
openJob/covstate andtoggleCoverage(dedupe/loading-lock/retry-on-error) are copied verbatim fromGlobalConversionPane.tsx. Two independent copies of this logic will silently diverge over time (e.g., a caching or error-message fix applied to one pane but not the other).Consider extracting this into a shared hook (e.g.
useCoverageDrawer(job)inconversionShared.tsx) that both panes consume.♻️ Sketch of shared hook extraction
+// conversionShared.tsx +export function useCoverageDrawer() { + const [openJob, setOpenJob] = useState<string | null>(null); + const [cov, setCov] = useState<Record<string, ConversionQualityMetricsResponse | { error: string } | "loading">>({}); + const toggleCoverage = useCallback(async (job: IfcReadyListItem) => { + if (!job.conversion_job_id) return; + const id = job.ifc_ready_job_id; + if (openJob === id) { setOpenJob(null); return; } + setOpenJob(id); + const cached = cov[id]; + if (cached === "loading") return; + if (cached && !("error" in cached)) return; + setCov((p) => ({ ...p, [id]: "loading" })); + try { + const r = await coordinatorClient.conversionQualityMetrics(job.conversion_job_id); + setCov((p) => ({ ...p, [id]: r })); + } catch (e) { + setCov((p) => ({ ...p, [id]: { error: `${t("未取得 coverage：", "Coverage not available: ")}${String(e)}` } })); + } + }, [openJob, cov]); + return { openJob, cov, toggleCoverage }; +}🤖 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/console/modelData/ObjectDetailPane.tsx` around lines 65 - 82, The coverage drawer state and toggle logic in ObjectDetailPane’s toggleCoverage are duplicated from GlobalConversionPane, so this should be centralized instead of maintained in two places. Extract the openJob/cov state, loading lock, cache reuse, and error handling into a shared hook or helper (for example a useCoverageDrawer hook in a shared conversion module), then have both ObjectDetailPane and GlobalConversionPane consume that shared implementation to keep behavior consistent.
🤖 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/console/modelData/useConversionActions.ts`:
- Around line 93-97: In confirmTrigger, the dialog is being closed before the
ledger refresh is confirmed because loadRecords() is fired-and-forgotten instead
of awaited. Update the flow in useConversionActions so confirmTrigger awaits
loadRecords() and only calls setPendingTriggerKey(null) after the refresh
succeeds; if loadRecords fails, keep the dialog open and surface an explicit
error, following the same evidence-based pattern used by runAction.
---
Outside diff comments:
In `@web-viewer-sample/src/console/pages.tsx`:
- Around line 379-418: The doRun callback reads sessions when deriving
expected_mapping_url for mapping enrichment, but sessions is missing from the
useCallback dependency array, so the callback can use a stale sessions snapshot.
Update the dependency list on doRun to include sessions (alongside the existing
state.step, state.runError, state.ifcPath, idsPath, and selectedSession
references) so the lookup stays in sync with current session data.
---
Nitpick comments:
In `@web-viewer-sample/src/console/modelData/GlobalConversionPane.tsx`:
- Around line 39-58: The `toggleCoverage` logic in `GlobalConversionPane` is
duplicated with the matching implementation in `ObjectDetailPane`, so extract
the shared open/collapse/cache/error flow into a reusable hook such as
`useCoverageDrawer` (similar to `useConversionActions`) and have both panes call
that single source of truth. Keep the hook responsible for the `openJob`/`cov`
state and the async `toggleCoverage` behavior so future changes only need to be
made once.
In `@web-viewer-sample/src/console/modelData/MinioTreePane.test.tsx`:
- Around line 26-33: The waitFor polling helper is duplicated across the
modelData test files, so extract the shared poll-plus-act retry logic from
MinioTreePane.test.tsx into a common test-utils module under modelData and
update GlobalConversionPane.test.tsx, ObjectDetailPane.test.tsx, and
ModelDataPage.test.tsx to import it. Keep the shared helper behavior identical
by preserving the same signature, act-based tick, assert retry loop, and
maxTicks default so all tests use one implementation and future tweaks stay
consistent.
In `@web-viewer-sample/src/console/modelData/MinioTreePane.tsx`:
- Around line 112-153: Move the ledger status lookup in MinioTreePane’s folder
object render so `ledgerChipStatus(idk, records, recordsIncomplete)` is only
called for `obj.role === "source_ifc"`, since the chip is only rendered there.
Keep the existing `ledgerChipStatus`, `MINIO_CHIP_LABEL`, and
`minio-chip-${idk}` logic together inside that branch so non-source_ifc rows
skip the extra lookup.
In `@web-viewer-sample/src/console/modelData/ObjectDetailPane.test.tsx`:
- Around line 59-69: The test fixture helpers in ObjectDetailPane.test.tsx
duplicate the same model-data factories used in ModelDataPage.test.tsx, so
extract the shared `K` constant and the
`makeObject`/`makeRecord`/`makeJob`/`makeData` helpers into a common model-data
test fixture module and import them from both specs. Keep the existing helper
names or re-export them so the other test files in this directory can reuse the
same fixtures and avoid copy/paste drift.
In `@web-viewer-sample/src/console/modelData/ObjectDetailPane.tsx`:
- Around line 65-82: The coverage drawer state and toggle logic in
ObjectDetailPane’s toggleCoverage are duplicated from GlobalConversionPane, so
this should be centralized instead of maintained in two places. Extract the
openJob/cov state, loading lock, cache reuse, and error handling into a shared
hook or helper (for example a useCoverageDrawer hook in a shared conversion
module), then have both ObjectDetailPane and GlobalConversionPane consume that
shared implementation to keep behavior consistent.
In `@web-viewer-sample/src/console/modelData/useMinioFolder.ts`:
- Around line 35-67: Add regression coverage for the `loadGenRef` race-guard in
`useMinioFolder`; the current `load` callback’s concurrent-request protection is
untested. Create a test alongside `useMinioFolder` using the existing
`createRoot`/`act`/`waitFor` pattern to simulate a slow earlier
`coordinatorClient.getMinioFolder` resolving after a faster later navigation,
and assert the stale response does not overwrite the newer folder state.
Reference the `load`, `loadGenRef`, and `coordinatorClient.getMinioFolder`
behavior directly in the test setup.
🪄 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: 7949f554-b21c-4bde-aedb-c12ebb0c7658
📒 Files selected for processing (20)
web-viewer-sample/src/console/A1CrossLinks.test.tsxweb-viewer-sample/src/console/A1ViewerEmbed.test.tsxweb-viewer-sample/src/console/EdgeConsole.aliasRedirect.test.tsxweb-viewer-sample/src/console/EdgeConsole.tsxweb-viewer-sample/src/console/console.test.tsxweb-viewer-sample/src/console/data.tsweb-viewer-sample/src/console/edge-console.cssweb-viewer-sample/src/console/modelData/GlobalConversionPane.test.tsxweb-viewer-sample/src/console/modelData/GlobalConversionPane.tsxweb-viewer-sample/src/console/modelData/MinioTreePane.test.tsxweb-viewer-sample/src/console/modelData/MinioTreePane.tsxweb-viewer-sample/src/console/modelData/ModelDataPage.test.tsxweb-viewer-sample/src/console/modelData/ModelDataPage.tsxweb-viewer-sample/src/console/modelData/ObjectDetailPane.test.tsxweb-viewer-sample/src/console/modelData/ObjectDetailPane.tsxweb-viewer-sample/src/console/modelData/useConversionActions.tsweb-viewer-sample/src/console/modelData/useConversionData.test.tsweb-viewer-sample/src/console/modelData/useConversionData.tsweb-viewer-sample/src/console/modelData/useMinioFolder.tsweb-viewer-sample/src/console/pages.tsx
| try { | ||
| await coordinatorClient.triggerConversion(pendingTriggerKey, { forceRetrigger: true }); | ||
| void loadRecords(); // ledger 真值對齊：重抓 ledger（main trigger 已 server-side 落帳） | ||
| setPendingTriggerKey(null); // 成功才關 dialog | ||
| } catch (e) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Dialog closes before ledger refresh is confirmed, unlike runAction's evidence-based pattern.
confirmTrigger fires void loadRecords() without awaiting it, then immediately closes the dialog (setPendingTriggerKey(null)). If the refresh fails, the user sees the dialog close (implying success) while the ledger view is still stale — runAction avoids exactly this by awaiting load() and keeping the dialog open with an explicit error when jobsOk/mwOk is false. Since loadRecords() returns Promise<void> (no success flag), confirmTrigger has no way to detect and surface a failed refresh the same way.
🔧 Suggested fix
- await coordinatorClient.triggerConversion(pendingTriggerKey, { forceRetrigger: true });
- void loadRecords(); // ledger 真值對齊：重抓 ledger（main trigger 已 server-side 落帳）
- setPendingTriggerKey(null); // 成功才關 dialog
+ await coordinatorClient.triggerConversion(pendingTriggerKey, { forceRetrigger: true });
+ await loadRecords(); // ledger 真值對齊：等待重抓完成再關 dialog
+ setPendingTriggerKey(null); // 成功才關 dialog📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| await coordinatorClient.triggerConversion(pendingTriggerKey, { forceRetrigger: true }); | |
| void loadRecords(); // ledger 真值對齊：重抓 ledger（main trigger 已 server-side 落帳） | |
| setPendingTriggerKey(null); // 成功才關 dialog | |
| } catch (e) { | |
| try { | |
| await coordinatorClient.triggerConversion(pendingTriggerKey, { forceRetrigger: true }); | |
| await loadRecords(); // ledger 真值對齊：等待重抓完成再關 dialog | |
| setPendingTriggerKey(null); // 成功才關 dialog | |
| } catch (e) { |
🤖 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/console/modelData/useConversionActions.ts` around lines
93 - 97, In confirmTrigger, the dialog is being closed before the ledger refresh
is confirmed because loadRecords() is fired-and-forgotten instead of awaited.
Update the flow in useConversionActions so confirmTrigger awaits loadRecords()
and only calls setPendingTriggerKey(null) after the refresh succeeds; if
loadRecords fails, keep the dialog open and surface an explicit error, following
the same evidence-based pattern used by runAction.
PR Review Agent Summary
Blockers
Warnings
Validation Commands
Checks
Human Review Notes
|
PR Review Agent Summary
Blockers
Warnings
Validation Commands
Checks
Human Review Notes
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67686ef124
ℹ️ 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".
| <Btn primary data-testid="a1-step-run" disabled={state.step === "idle" || !state.ifcPath || (state.step === "running" && !state.runError)} | ||
| caption={state.ifcPath ? "POST /api/governance/rule-runs" : t("先選定 IFC 模型；不需要 review session 即可檢核", "Select an IFC model first; review session is not required for validation")} onClick={doRun}> | ||
| caption={state.ifcPath ? (selectedSession ? "POST /api/governance/rule-runs/for-session/:sessionId" : "POST /api/governance/rule-runs") : t("先選定 IFC 模型", "Select an IFC model first")} onClick={doRun}> |
There was a problem hiding this comment.
When the operator follows the new MinIO-backed flow (select a MinIO object, then select the review session that can resolve the server-local IFC path), this button still requires state.ifcPath. The MinIO picker never dispatches PICK_FILE and its pick button is disabled, so createRuleRunForSession is unreachable unless the user first picks an unrelated local_fs file; that blocks the advertised MinIO → session → A1 validation path.
Useful? React with 👍 / 👎.
| <Btn | ||
| data-testid="md-detail-trigger" | ||
| caption="POST /api/conversion/trigger" | ||
| disabled={!["untracked", "failed", "indeterminate"].includes(chip)} |
There was a problem hiding this comment.
With chip === "indeterminate" (records are truncated or failed to load), this button still opens the shared trigger dialog, whose confirm path sends { force_retrigger: true }. The coordinator route treats that flag as a new salted attempt, so if the hidden ledger row is actually queued/ready this creates an unnecessary duplicate conversion instead of preserving idempotency; keep this state read-only or use the non-forced trigger until the ledger status is known.
Useful? React with 👍 / 👎.
| </Panel> | ||
|
|
||
| {/* coverage / 品質區：job 有 conversion_job_id → coverage 展開＋CoverageDrawer ＋ IN 品質誠實三行 */} | ||
| {job?.conversion_job_id && ( |
There was a problem hiding this comment.
After a coordinator restart, the ifc-ready job list can be empty while the persistent ledger record still has conversion_job_id/coverage_report for this selected model. Because the coverage/quality panel is gated only on the volatile job, those completed conversions lose their coverage evidence in the merged Model Data page even though the old ledger source still has it; fall back to the ledger record instead of hiding the panel.
Useful? React with 👍 / 👎.
…IA (#419) 7/7 task 終局處置(#259/#265/#303/#304 落地或 supersede,零新 production code); 兩支 stale E2E 由 #/conv 舊 IA 改 #/minio GlobalConversionPane 並實跑 2 passed (watch spec 補 attempt-local ledger 防舊帳假失敗);delta 依 main 現實修訂五處; proposal Status→closeout reconciled(可 archive)。 Claude-Session: https://claude.ai/code/session_01Bi4ujiFajBbd4nRhayJFxe Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
四個 canonical(sanitize 佇列表權威面移 #/minio、coverage/prioritize-retry 元件名與 雙頁誠實子句、fileserver 選擇器 route #/a1→#/issues)+五支 E2E(sanitize/coverage goto #/minio、五處 conv-refresh locator、fileserver 刪退役 file-library test)。 零 production code;sanitize E2E 修後實跑 1 passed;validate --all --strict 69/69。 Claude-Session: https://claude.ai/code/session_01Bi4ujiFajBbd4nRhayJFxe Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
#minio/ ModelDataPage 路徑，保留舊 hash alias 導向。rule-runs/for-session解析 server-local IFC path；A1 結果可一鍵開3D Review Roomhandoff，不再被未選 session gate 卡住。Test Plan
git diff --checknpm test -- --run src/console/A1ViewerEmbed.test.tsx src/console/A1CrossLinks.test.tsx src/console/console.test.tsx src/console/incomingHandoff.test.tsx-> 127 tests passednpm run build-> passed, with existing Vite >500 kB chunk warninggitnexus detect-changes --repo AI-BIM-governance --scope compare --base-ref main-> high risk: 28 files, 37 symbols, 9 flowspowershell -NoProfile -ExecutionPolicy Bypass -File scripts/pr-review-agent.ps1 -BaseSha <merge-base> -HeadSha HEAD -PrNumber 303 -RunId local-303 -OutputDir artifacts/tmp-pr-review-agent-local -SkipCommandExecution -SkipGitNexus -AllowGitNexusUnavailable-> warning/medium, missing formal evidence blocker clearedFrontend Verification
web-viewer-samplehash console routes:#minio, legacy#convalias, legacy#intakealias,#a1, and#review.a1-step-run,a1-open-review-room,a1-link-minio,a1-link-sessions, anda1-trigger-convert; console route tests covered alias/render paths.松風庵/root/main/u1/model.ifc, mockedreview_session_x, mocked rule-run failures withifc_guidandusd_prim_path.rule-runs/for-sessionwhen a review session is selected; A1 failure result opens Review Room with non-secret handoff context; Model Data routes render without falling back to removed pages.D:/Users/deploy/AI-bim-geowas not rebuilt; Vite still reports the existing chunk-size warning; full 3D highlight still needs Review Room runtime evidence with session, first frame, DataChannel, stage match, and mapping path observed.Deploy Path Verification
governance-serviceandweb-viewer-sample; no Docker, Kit runtime, env, or port contract change is claimed../scripts/dev/rebuild-test-deploy.ps1 -Buildfrom repo root when explicitly requested.-Buildhelper and forbids substituting-DryRun, so no deploy dry-run result is claimed.git diff --check, focused Vitest console suites,npm run build, localpr-review-agent, and GitNexusdetect-changes; deployment rebuild was not run.AI Coding Governance
docs/superpowers/specs/2026-07-06-model-data-conversion-merge-design.mdformal spec evidence,docs/plans/2026-07-06-model-data-conversion-merge.md,docs/plans/2026-07-06-model-data-conversion-merge-design.md, and user-requested A1 MinIO / Review Room correction.gitnexus detect-changes --repo AI-BIM-governance --scope compare --base-ref mainreturned high risk: 28 files, 37 symbols, 9 flows.AGENTS.md/CLAUDE.mddoc edits from earlier commits, so governance checks should review them.pr-review-agentandagent-governanceperdocs/agents/github-workflow.md.