feat(viewer): refresh button to reload the open model from disk (#1345)#1352
Merged
Conversation
Add a Refresh button that re-reads the currently open model from disk and re-parses it, so a model can be monitored during design without closing and re-opening the same file. A `<input type="file">` File is a frozen snapshot of the bytes at pick time, so re-reading it can never reflect on-disk edits. Only the File System Access API (showOpenFilePicker -> FileSystemFileHandle.getFile()) returns a fresh snapshot. The Open button now prefers that API when available (Chromium, secure context) to capture a live handle, falling back to `<input type="file">` elsewhere. - services/file-system-access.ts: feature-detected open + permission-checked fresh re-read helpers. - types/file-system-access.d.ts: ambient decls for the API surface missing from TS lib.dom (showOpenFilePicker, options, queryPermission/requestPermission). - FederatedModel.sourceHandle: in-memory live handle (never serialized to cache). - loadFile() persists the handle on the primary model record. - MainToolbar: Refresh shown only for a single model that has a live handle (drag-drop / input / cache-restored / federated loads have none).
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds File System Access API support to viewer file open, add, drop, and refresh flows. Live file handles are propagated into model records, recent-file caching gains filename lookup, and command palette actions are adjusted to preserve browser file-dialog activation. ChangesFile System Access API Integration and Refresh
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
Broaden the refresh feature so it works regardless of how a model was opened, and refresh whole federations at once. The first PR only captured a live handle from the toolbar Open button, so a model opened from the empty-state "Open .ifc file" button, drag-drop, or the Add-Model button had no handle and showed no Refresh button. Now every in-session ingestion path on Chromium captures a handle: - ViewportContainer empty-state open buttons -> showOpenFilePicker. - Drag-drop -> DataTransferItem.getAsFileSystemHandle() (kicked off synchronously before any await, since the DataTransferItemList is neutered once the drop handler returns). - Add-Model button + multi-file Open -> picker handles threaded through loadFilesSequentially -> addModel -> the federated finalize. Refresh is now "Refresh all": it re-reads every handle-backed model's bytes, then reloads a single model in place or rebuilds the federation (preserving id, order, visibility, collapsed), reusing the proven reload sequence. Reads happen before anything is cleared, so a failed read never empties the viewer; partial failures are reported per file. The button shows only when every loaded model has a handle (session-only, in memory), so a mixed session never risks dropping handle-less models. No polling/auto-watch and no cross-session persistence, per the agreed scope.
…1345) `runCommand` deferred every action to the next animation frame, and the recent-file action then awaited an IndexedDB read. Both discard the transient user activation Chrome requires to open a file dialog, so "Open File" and the recent-file cache-miss fallback silently no-opped. (A cache HIT still loaded, since dispatching a load event needs no activation, which is why it looked intermittent.) - Command.immediate runs file-dialog actions synchronously inside the click, preserving activation. - Recent-file clicks decide cache hit/miss from a key set pre-loaded on open (getCachedFileNames), so no await sits between the gesture and the picker: hit -> load the cached blob; miss -> re-pick. - "Open File" and the re-pick path dispatch `ifc-lite:open-files`, which the (always-mounted) MainToolbar routes to its handle-capturing open. So palette opens now work AND capture a live handle, keeping the model refreshable.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
apps/viewer/src/lib/recent-files.ts (1)
126-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the swallowed error and close the DB on the failure path.
The
catchsilently discards the error, which makes IndexedDB issues invisible during debugging. Additionally,db.close()(Line 134) is only reached on success — ifgetAllKeys()rejects, control jumps tocatchand the opened connection is never closed, leaking it. Wrap cleanup infinallyand log before returning the fallback.♻️ Proposed fix
export async function getCachedFileNames(): Promise<string[]> { + let db: IDBDatabase | undefined; try { - const db = await openDB(); + db = await openDB(); const tx = db.transaction(STORE_NAME, 'readonly'); const req = tx.objectStore(STORE_NAME).getAllKeys(); const keys = await new Promise<IDBValidKey[]>((resolve, reject) => { req.onsuccess = () => resolve(req.result); req.onerror = () => reject(req.error); }); - db.close(); return keys.map(String); - } catch { + } catch (err) { + console.error('[recent-files] getCachedFileNames failed:', err); return []; + } finally { + db?.close(); } }As per coding guidelines: "No silent
catch {}— log or rethrow errors".🤖 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 `@apps/viewer/src/lib/recent-files.ts` around lines 126 - 138, The recent-files IndexedDB read path swallows failures and only closes the connection on success, so update the `openDB()`/`getAllKeys()` flow in `recent-files.ts` to log the caught error instead of using a silent `catch`, and move `db.close()` into a `finally` so it always runs even when `tx.objectStore(STORE_NAME).getAllKeys()` rejects. Keep the fallback return of an empty array after logging, and preserve the existing `keys.map(String)` success path.Source: Coding guidelines
🤖 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 `@apps/viewer/src/components/viewer/MainToolbar.tsx`:
- Around line 445-446: The IFCX handling in MainToolbar is inconsistent because
isSupportedModelFile now accepts case-insensitive filenames, but later logic
still checks raw string suffixes like .endsWith('.ifcx'), causing MODEL.IFCX to
skip IFCX-specific composition and overlay behavior. Update the downstream IFCX
detection in MainToolbar (including the branches around the supportedFiles flow
and the later composition/overlay checks) to normalize filenames before testing
suffixes, and use a single shared IFCX check so MODEL.IFCX is treated the same
regardless of case.
- Around line 937-947: The icon-only Refresh button in MainToolbar lacks an
accessible name, so add an aria-label directly on the Button used with RefreshCw
and handleRefresh. Keep the tooltip if desired, but ensure assistive tech can
identify the action from the button itself by giving it a clear label such as
“Refresh”.
- Around line 583-618: The refresh flow in MainToolbar’s reload handler is not
atomic: the multi-model path clears the viewer and reloads only the fresh
entries, which drops failed models, and the single-model path shows success
before loadFile finishes. Update the reload logic so the existing scene/state is
only replaced after all reload operations complete successfully, and keep failed
models untouched when any read fails. Use the existing symbols ok, failedNames,
loadFile, addModel, and clearAllModels to locate and adjust the refresh branch,
and move the success toast to fire only after the async reload work has fully
completed.
- Around line 536-557: The file picker flow in MainToolbar lets unsupported “all
files” selections through, so sanitize the results before any downstream work.
In the openIfcFilesWithHandles/handleFileSelect path, filter opened to only the
supported IFC/IFCX files before calling recordRecentFiles, cacheFileBlobs,
loadFile, or loadFilesSequentially, or tighten the picker configuration to
accept only the same formats as the input/Add Model flows.
In `@apps/viewer/src/components/viewer/ViewportContainer.tsx`:
- Around line 487-493: The drop handling in ViewportContainer is pairing files
to handles by filename, which collapses same-named files from different folders
and can assign the wrong sourceHandle. Update the logic around
handlesFromDataTransfer, supportedFiles, and the refresh path to preserve the
original captured handle/file ordering or direct pairing instead of using
handleByName. Ensure each dropped file keeps its own corresponding handle so
Refresh rereads the correct disk file.
In `@apps/viewer/src/services/file-system-access.ts`:
- Around line 137-140: The dropped-handle fallback in getAsFileSystemHandle is
silently swallowing exceptions, which hides drag/drop failures. Update the
try/catch in file-system-access.ts so the catch block either logs the caught
error with enough context or rethrows it instead of returning a silent null, and
keep the existing getAsFileSystemHandle() path behavior unchanged when it
succeeds.
In `@apps/viewer/src/store/types.ts`:
- Around line 339-344: Update the `sourceHandle` lifecycle documentation in
`types.ts` so it matches the new federation behavior:
`useIfcFederation.loadFilesSequentially()` can now attach live handles to
federated models for refresh support. Adjust the comment on `sourceHandle` to
exclude only non-handle federated loads, while still noting it is absent for
drag-drop, `<input type="file">`, cache-restored, and any federated load created
without a live handle.
---
Nitpick comments:
In `@apps/viewer/src/lib/recent-files.ts`:
- Around line 126-138: The recent-files IndexedDB read path swallows failures
and only closes the connection on success, so update the
`openDB()`/`getAllKeys()` flow in `recent-files.ts` to log the caught error
instead of using a silent `catch`, and move `db.close()` into a `finally` so it
always runs even when `tx.objectStore(STORE_NAME).getAllKeys()` rejects. Keep
the fallback return of an empty array after logging, and preserve the existing
`keys.map(String)` success path.
🪄 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: 0f4aba38-bb67-4697-b572-2e638ba7f479
📒 Files selected for processing (9)
apps/viewer/src/components/viewer/CommandPalette.tsxapps/viewer/src/components/viewer/MainToolbar.tsxapps/viewer/src/components/viewer/ViewportContainer.tsxapps/viewer/src/hooks/useIfcFederation.tsapps/viewer/src/hooks/useIfcLoader.tsapps/viewer/src/lib/recent-files.tsapps/viewer/src/services/file-system-access.tsapps/viewer/src/store/types.tsapps/viewer/src/types/file-system-access.d.ts
Comment on lines
+445
to
+446
| // Filter to supported files (IFC, IFCX, GLB, point clouds) | ||
| const supportedFiles = Array.from(files).filter(isSupportedModelFile); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize IFCX detection after accepting case-insensitive filenames.
isSupportedModelFile accepts MODEL.IFCX, but the raw .endsWith('.ifcx') checks route it as non-IFCX, skipping IFCX composition/overlay behavior.
Suggested fix
function isSupportedModelFile(f: File): boolean {
const n = f.name.toLowerCase();
return n.endsWith('.ifc') || n.endsWith('.ifcx') || n.endsWith('.glb')
|| n.endsWith('.las') || n.endsWith('.laz') || n.endsWith('.ply') || n.endsWith('.pcd')
|| n.endsWith('.e57') || n.endsWith('.pts') || n.endsWith('.xyz');
}
+
+function isIfcxModelFile(f: File): boolean {
+ return f.name.toLowerCase().endsWith('.ifcx');
+}
- const newFilesAreIfcx = supportedFiles.every(f => f.name.endsWith('.ifcx'));
+ const newFilesAreIfcx = supportedFiles.every(isIfcxModelFile);
- const allIfcx = files.every(f => f.name.endsWith('.ifcx'));
+ const allIfcx = files.every(isIfcxModelFile);Also applies to: 487-488, 548-548
🤖 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 `@apps/viewer/src/components/viewer/MainToolbar.tsx` around lines 445 - 446,
The IFCX handling in MainToolbar is inconsistent because isSupportedModelFile
now accepts case-insensitive filenames, but later logic still checks raw string
suffixes like .endsWith('.ifcx'), causing MODEL.IFCX to skip IFCX-specific
composition and overlay behavior. Update the downstream IFCX detection in
MainToolbar (including the branches around the supportedFiles flow and the later
composition/overlay checks) to normalize filenames before testing suffixes, and
use a single shared IFCX check so MODEL.IFCX is treated the same regardless of
case.
Comment on lines
+536
to
+557
| const opened = await openIfcFilesWithHandles(); | ||
| if (!opened) return; // cancelled, unavailable, or picker failed | ||
|
|
||
| const files = opened.map(o => o.file); | ||
| recordRecentFiles(files.map(f => ({ name: f.name, size: f.size }))); | ||
| void cacheFileBlobs(files); | ||
|
|
||
| if (opened.length === 1) { | ||
| // Single model: keep the handle so Refresh can re-read it from disk. | ||
| void loadFile(opened[0].file, { kind: 'primary' }, { sourceHandle: opened[0].handle }); | ||
| } else { | ||
| // Multiple files mirror handleFileSelect's branching. | ||
| const allIfcx = files.every(f => f.name.endsWith('.ifcx')); | ||
| resetViewerState(); | ||
| clearAllModels(); | ||
| if (allIfcx) { | ||
| // IFCX layers compose into one shared store — no per-file handle. | ||
| void loadFederatedIfcx(files); | ||
| } else { | ||
| // Carry each file's handle so the whole federation stays refreshable. | ||
| void loadFilesSequentially(files, opened.map(o => o.handle)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🌐 Web query:
Does showOpenFilePicker with excludeAcceptAllOption: false allow users to choose files outside the declared accept types?
💡 Result:
Yes—setting excludeAcceptAllOption: false means the picker includes an “all files / no filter” option, so users can select files outside the declared accept types (when that “all files” option is available). [1][2] Specifically, the File System Access API defines excludeAcceptAllOption as controlling whether the picker “include[s] an option to not apply any file type filters.” By default it is false, so the “not apply any file type filters” option is present; setting it to true removes that option. [1][2] If you want to prevent selecting outside your declared types, set excludeAcceptAllOption: true. [1][2]
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Window/showOpenFilePicker
- 2: https://wicg.github.io/file-system-access/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file(s) and search for the picker/filters used in the viewer code.
git ls-files 'apps/viewer/src/**/*' | rg 'MainToolbar\.tsx|openIfcFilesWithHandles|isSupportedModelFile|isIfcxModelFile|loadFile|loadFilesSequentially|handleFileSelect|showOpenFilePicker'Repository: LTplus-AG/ifc-lite
Length of output: 206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant source around the picker flow and helper functions.
files=$(git ls-files 'apps/viewer/src/**/*' | rg 'MainToolbar\.tsx|openIfcFilesWithHandles|isSupportedModelFile|isIfcxModelFile|loadFile|loadFilesSequentially|handleFileSelect')
printf '%s\n' "$files"
for f in $files; do
echo "--- $f ---"
wc -l "$f"
done
# Show AST outline for the main toolbar file if present.
if git ls-files 'apps/viewer/src/components/viewer/MainToolbar.tsx' >/dev/null 2>&1; then
ast-grep outline apps/viewer/src/components/viewer/MainToolbar.tsx --view expanded
fiRepository: LTplus-AG/ifc-lite
Length of output: 1650
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the picker helper and the toolbar branch around the reported lines.
sed -n '280,360p' apps/viewer/src/components/viewer/MainToolbar.tsx
printf '\n---\n'
sed -n '520,580p' apps/viewer/src/components/viewer/MainToolbar.tsx
printf '\n---\n'
rg -n "openIfcFilesWithHandles|showOpenFilePicker|excludeAcceptAllOption|isIfcxModelFile|loadFilesSequentially|loadFile\\(" apps/viewer/src/components/viewer/MainToolbar.tsxRepository: LTplus-AG/ifc-lite
Length of output: 7702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding implementation for the picker flow and model-file filtering.
sed -n '280,360p' apps/viewer/src/components/viewer/MainToolbar.tsx
printf '\n---\n'
sed -n '520,580p' apps/viewer/src/components/viewer/MainToolbar.tsx
printf '\n---\n'
rg -n "openIfcFilesWithHandles|showOpenFilePicker|excludeAcceptAllOption|isIfcxModelFile|loadFilesSequentially|loadFile\\(" apps/viewer/src/components/viewer/MainToolbar.tsxRepository: LTplus-AG/ifc-lite
Length of output: 7702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant implementation details for the file-picker branch.
sed -n '280,360p' apps/viewer/src/components/viewer/MainToolbar.tsx
printf '\n---\n'
sed -n '520,580p' apps/viewer/src/components/viewer/MainToolbar.tsx
printf '\n---\n'
rg -n "openIfcFilesWithHandles|showOpenFilePicker|excludeAcceptAllOption|isIfcxModelFile|loadFilesSequentially|loadFile\\(" apps/viewer/src/components/viewer/MainToolbar.tsxRepository: LTplus-AG/ifc-lite
Length of output: 7702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the picker flow and nearby helper functions.
sed -n '280,360p' apps/viewer/src/components/viewer/MainToolbar.tsx
printf '\n---\n'
sed -n '520,580p' apps/viewer/src/components/viewer/MainToolbar.tsx
printf '\n---\n'
rg -n "openIfcFilesWithHandles|showOpenFilePicker|excludeAcceptAllOption|isIfcxModelFile|loadFilesSequentially|loadFile\\(" apps/viewer/src/components/viewer/MainToolbar.tsxRepository: LTplus-AG/ifc-lite
Length of output: 7702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the File System Access helper to see what picker options it uses.
rg -n "function openIfcFilesWithHandles|const openIfcFilesWithHandles|showOpenFilePicker|excludeAcceptAllOption|accept:" apps/viewer/srcRepository: LTplus-AG/ifc-lite
Length of output: 1701
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the File System Access helper to see what picker options it uses.
rg -n "function openIfcFilesWithHandles|const openIfcFilesWithHandles|showOpenFilePicker|excludeAcceptAllOption|accept:" apps/viewer/srcRepository: LTplus-AG/ifc-lite
Length of output: 1701
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the File System Access helper to see what picker options it uses.
rg -n "function openIfcFilesWithHandles|const openIfcFilesWithHandles|showOpenFilePicker|excludeAcceptAllOption|accept:" apps/viewer/srcRepository: LTplus-AG/ifc-lite
Length of output: 1701
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' apps/viewer/src/services/file-system-access.ts
printf '\n---\n'
sed -n '430,510p' apps/viewer/src/components/viewer/MainToolbar.tsxRepository: LTplus-AG/ifc-lite
Length of output: 9482
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' apps/viewer/src/services/file-system-access.ts
printf '\n---\n'
sed -n '430,510p' apps/viewer/src/components/viewer/MainToolbar.tsxRepository: LTplus-AG/ifc-lite
Length of output: 9482
Filter unsupported picker results before loading. The File System Access picker still allows an “all files” choice here (excludeAcceptAllOption: false), so unsupported files can reach recordRecentFiles, cacheFileBlobs, and loadFile/loadFilesSequentially. Filter opened first, or make the picker accept-only, to match the <input> and Add Model paths.
🤖 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 `@apps/viewer/src/components/viewer/MainToolbar.tsx` around lines 536 - 557,
The file picker flow in MainToolbar lets unsupported “all files” selections
through, so sanitize the results before any downstream work. In the
openIfcFilesWithHandles/handleFileSelect path, filter opened to only the
supported IFC/IFCX files before calling recordRecentFiles, cacheFileBlobs,
loadFile, or loadFilesSequentially, or tighten the picker configuration to
accept only the same formats as the input/Add Model flows.
Comment on lines
+583
to
+618
| const ok = reads.filter((r) => r.fresh) as { model: typeof targets[number]; fresh: File }[]; | ||
| const failedNames = reads.filter((r) => !r.fresh).map((r) => `"${r.model.name}"`); | ||
|
|
||
| if (ok.length === 0) { | ||
| toast.error(`Couldn't re-read ${failedNames.join(', ')}. Files may have moved, been deleted, or access was denied.`); | ||
| return; | ||
| } | ||
|
|
||
| recordRecentFiles(ok.map((r) => ({ name: r.fresh.name, size: r.fresh.size }))); | ||
| void cacheFileBlobs(ok.map((r) => r.fresh)); | ||
|
|
||
| if (targets.length === 1) { | ||
| void loadFile(ok[0].fresh, { kind: 'primary' }, { sourceHandle: ok[0].model.sourceHandle }); | ||
| } else { | ||
| // Rebuild the federation from fresh bytes, preserving id + order + state. | ||
| clearAllModels(); | ||
| for (const r of ok) { | ||
| const reloadedId = await addModel(r.fresh, { | ||
| name: r.model.name, | ||
| modelId: r.model.id, | ||
| loadedAt: r.model.loadedAt, | ||
| visible: r.model.visible, | ||
| collapsed: r.model.collapsed, | ||
| sourceHandle: r.model.sourceHandle, | ||
| }); | ||
| if (reloadedId && r.model.visible === false) { | ||
| useViewerStore.getState().setModelVisibility(r.model.id, false); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (failedNames.length > 0) { | ||
| toast.error(`Refreshed ${ok.length}; couldn't re-read ${failedNames.join(', ')}.`); | ||
| } else { | ||
| toast.success(ok.length === 1 ? `Refreshed "${ok[0].fresh.name}"` : `Refreshed ${ok.length} models`); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep Refresh atomic and report success only after reload completes.
On partial read failure, the multi-model branch clears the scene and reloads only ok, dropping failed models. The single-model branch also fires a success toast before loadFile finishes.
Suggested fix
- const ok = reads.filter((r) => r.fresh) as { model: typeof targets[number]; fresh: File }[];
const failedNames = reads.filter((r) => !r.fresh).map((r) => `"${r.model.name}"`);
- if (ok.length === 0) {
+ if (failedNames.length > 0) {
toast.error(`Couldn't re-read ${failedNames.join(', ')}. Files may have moved, been deleted, or access was denied.`);
return;
}
+ const ok = reads as { model: typeof targets[number]; fresh: File }[];
recordRecentFiles(ok.map((r) => ({ name: r.fresh.name, size: r.fresh.size })));
void cacheFileBlobs(ok.map((r) => r.fresh));
- if (targets.length === 1) {
- void loadFile(ok[0].fresh, { kind: 'primary' }, { sourceHandle: ok[0].model.sourceHandle });
- } else {
- // Rebuild the federation from fresh bytes, preserving id + order + state.
- clearAllModels();
- for (const r of ok) {
- const reloadedId = await addModel(r.fresh, {
- name: r.model.name,
- modelId: r.model.id,
- loadedAt: r.model.loadedAt,
- visible: r.model.visible,
- collapsed: r.model.collapsed,
- sourceHandle: r.model.sourceHandle,
- });
- if (reloadedId && r.model.visible === false) {
- useViewerStore.getState().setModelVisibility(r.model.id, false);
+ try {
+ if (targets.length === 1) {
+ await loadFile(ok[0].fresh, { kind: 'primary' }, { sourceHandle: ok[0].model.sourceHandle });
+ } else {
+ // Rebuild the federation from fresh bytes, preserving id + order + state.
+ clearAllModels();
+ for (const r of ok) {
+ const reloadedId = await addModel(r.fresh, {
+ name: r.model.name,
+ modelId: r.model.id,
+ loadedAt: r.model.loadedAt,
+ visible: r.model.visible,
+ collapsed: r.model.collapsed,
+ sourceHandle: r.model.sourceHandle,
+ });
+ if (reloadedId && r.model.visible === false) {
+ useViewerStore.getState().setModelVisibility(r.model.id, false);
+ }
}
}
+ } catch (err) {
+ console.error('[MainToolbar] Refresh reload failed', err);
+ toast.error(`Refresh failed: ${err instanceof Error ? err.message : 'Unknown error'}`);
+ return;
}
- if (failedNames.length > 0) {
- toast.error(`Refreshed ${ok.length}; couldn't re-read ${failedNames.join(', ')}.`);
- } else {
- toast.success(ok.length === 1 ? `Refreshed "${ok[0].fresh.name}"` : `Refreshed ${ok.length} models`);
- }
+ toast.success(ok.length === 1 ? `Refreshed "${ok[0].fresh.name}"` : `Refreshed ${ok.length} models`);📝 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.
Suggested change
| const ok = reads.filter((r) => r.fresh) as { model: typeof targets[number]; fresh: File }[]; | |
| const failedNames = reads.filter((r) => !r.fresh).map((r) => `"${r.model.name}"`); | |
| if (ok.length === 0) { | |
| toast.error(`Couldn't re-read ${failedNames.join(', ')}. Files may have moved, been deleted, or access was denied.`); | |
| return; | |
| } | |
| recordRecentFiles(ok.map((r) => ({ name: r.fresh.name, size: r.fresh.size }))); | |
| void cacheFileBlobs(ok.map((r) => r.fresh)); | |
| if (targets.length === 1) { | |
| void loadFile(ok[0].fresh, { kind: 'primary' }, { sourceHandle: ok[0].model.sourceHandle }); | |
| } else { | |
| // Rebuild the federation from fresh bytes, preserving id + order + state. | |
| clearAllModels(); | |
| for (const r of ok) { | |
| const reloadedId = await addModel(r.fresh, { | |
| name: r.model.name, | |
| modelId: r.model.id, | |
| loadedAt: r.model.loadedAt, | |
| visible: r.model.visible, | |
| collapsed: r.model.collapsed, | |
| sourceHandle: r.model.sourceHandle, | |
| }); | |
| if (reloadedId && r.model.visible === false) { | |
| useViewerStore.getState().setModelVisibility(r.model.id, false); | |
| } | |
| } | |
| } | |
| if (failedNames.length > 0) { | |
| toast.error(`Refreshed ${ok.length}; couldn't re-read ${failedNames.join(', ')}.`); | |
| } else { | |
| toast.success(ok.length === 1 ? `Refreshed "${ok[0].fresh.name}"` : `Refreshed ${ok.length} models`); | |
| } | |
| const failedNames = reads.filter((r) => !r.fresh).map((r) => `"${r.model.name}"`); | |
| if (failedNames.length > 0) { | |
| toast.error(`Couldn't re-read ${failedNames.join(', ')}. Files may have moved, been deleted, or access was denied.`); | |
| return; | |
| } | |
| const ok = reads as { model: typeof targets[number]; fresh: File }[]; | |
| recordRecentFiles(ok.map((r) => ({ name: r.fresh.name, size: r.fresh.size }))); | |
| void cacheFileBlobs(ok.map((r) => r.fresh)); | |
| try { | |
| if (targets.length === 1) { | |
| await loadFile(ok[0].fresh, { kind: 'primary' }, { sourceHandle: ok[0].model.sourceHandle }); | |
| } else { | |
| // Rebuild the federation from fresh bytes, preserving id + order + state. | |
| clearAllModels(); | |
| for (const r of ok) { | |
| const reloadedId = await addModel(r.fresh, { | |
| name: r.model.name, | |
| modelId: r.model.id, | |
| loadedAt: r.model.loadedAt, | |
| visible: r.model.visible, | |
| collapsed: r.model.collapsed, | |
| sourceHandle: r.model.sourceHandle, | |
| }); | |
| if (reloadedId && r.model.visible === false) { | |
| useViewerStore.getState().setModelVisibility(r.model.id, false); | |
| } | |
| } | |
| } | |
| } catch (err) { | |
| console.error('[MainToolbar] Refresh reload failed', err); | |
| toast.error(`Refresh failed: ${err instanceof Error ? err.message : 'Unknown error'}`); | |
| return; | |
| } | |
| toast.success(ok.length === 1 ? `Refreshed "${ok[0].fresh.name}"` : `Refreshed ${ok.length} models`); |
🤖 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 `@apps/viewer/src/components/viewer/MainToolbar.tsx` around lines 583 - 618,
The refresh flow in MainToolbar’s reload handler is not atomic: the multi-model
path clears the viewer and reloads only the fresh entries, which drops failed
models, and the single-model path shows success before loadFile finishes. Update
the reload logic so the existing scene/state is only replaced after all reload
operations complete successfully, and keep failed models untouched when any read
fails. Use the existing symbols ok, failedNames, loadFile, addModel, and
clearAllModels to locate and adjust the refresh branch, and move the success
toast to fire only after the async reload work has fully completed.
Comment on lines
+937
to
+947
| <Button | ||
| variant="ghost" | ||
| size="icon-sm" | ||
| onClick={(e) => { | ||
| (e.currentTarget as HTMLButtonElement).blur(); | ||
| void handleRefresh(); | ||
| }} | ||
| disabled={loading} | ||
| > | ||
| <RefreshCw className="h-4 w-4" /> | ||
| </Button> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add an accessible name to the icon-only Refresh button.
The new button has only an SVG icon and tooltip; give it an aria-label so assistive tech can identify the action.
Suggested fix
<Button
variant="ghost"
size="icon-sm"
+ aria-label={models.size > 1 ? 'Refresh models from disk' : 'Refresh model from disk'}
onClick={(e) => {
(e.currentTarget as HTMLButtonElement).blur();
void handleRefresh();
}}📝 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.
Suggested change
| <Button | |
| variant="ghost" | |
| size="icon-sm" | |
| onClick={(e) => { | |
| (e.currentTarget as HTMLButtonElement).blur(); | |
| void handleRefresh(); | |
| }} | |
| disabled={loading} | |
| > | |
| <RefreshCw className="h-4 w-4" /> | |
| </Button> | |
| <Button | |
| variant="ghost" | |
| size="icon-sm" | |
| aria-label={models.size > 1 ? 'Refresh models from disk' : 'Refresh model from disk'} | |
| onClick={(e) => { | |
| (e.currentTarget as HTMLButtonElement).blur(); | |
| void handleRefresh(); | |
| }} | |
| disabled={loading} | |
| > | |
| <RefreshCw className="h-4 w-4" /> | |
| </Button> |
🤖 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 `@apps/viewer/src/components/viewer/MainToolbar.tsx` around lines 937 - 947,
The icon-only Refresh button in MainToolbar lacks an accessible name, so add an
aria-label directly on the Button used with RefreshCw and handleRefresh. Keep
the tooltip if desired, but ensure assistive tech can identify the action from
the button itself by giving it a clear label such as “Refresh”.
Comment on lines
+487
to
+493
| // Capture live handles synchronously — the DataTransferItemList is neutered | ||
| // once this handler returns, so this must run before any await. | ||
| const handlesPromise = handlesFromDataTransfer(e.dataTransfer); | ||
|
|
||
| // Filter to supported files (IFC, IFCX, GLB, point clouds) | ||
| const allDropped = Array.from(e.dataTransfer.files); | ||
| const supportedFiles = allDropped.filter( | ||
| f => f.name.endsWith('.ifc') || f.name.endsWith('.ifcx') || f.name.endsWith('.glb') | ||
| || f.name.toLowerCase().endsWith('.las') || f.name.toLowerCase().endsWith('.laz') || f.name.toLowerCase().endsWith('.ply') || f.name.toLowerCase().endsWith('.pcd') || f.name.toLowerCase().endsWith('.e57') || f.name.toLowerCase().endsWith('.pts') || f.name.toLowerCase().endsWith('.xyz') | ||
| ); | ||
| const supportedFiles = allDropped.filter(isSupportedFile); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the captured handle/file pairing without a filename map.
handleByName collapses same-named files from different folders, so multiple dropped models can receive the same sourceHandle; Refresh would then re-read the wrong disk file.
Suggested fix
- void handlesPromise.then((opened) => {
- const handleByName = new Map((opened ?? []).map((o) => [o.file.name, o.handle]));
- routeLoad(supportedFiles, supportedFiles.map((f) => handleByName.get(f.name)));
- });
+ void handlesPromise
+ .then((opened) => {
+ const supportedOpened = (opened ?? []).filter((o) => isSupportedFile(o.file));
+ if (supportedOpened.length === supportedFiles.length) {
+ routeLoad(
+ supportedOpened.map((o) => o.file),
+ supportedOpened.map((o) => o.handle),
+ );
+ return;
+ }
+ routeLoad(supportedFiles);
+ })
+ .catch(() => routeLoad(supportedFiles));Also applies to: 509-512
🤖 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 `@apps/viewer/src/components/viewer/ViewportContainer.tsx` around lines 487 -
493, The drop handling in ViewportContainer is pairing files to handles by
filename, which collapses same-named files from different folders and can assign
the wrong sourceHandle. Update the logic around handlesFromDataTransfer,
supportedFiles, and the refresh path to preserve the original captured
handle/file ordering or direct pairing instead of using handleByName. Ensure
each dropped file keeps its own corresponding handle so Refresh rereads the
correct disk file.
Comment on lines
+137
to
+140
| try { | ||
| return item.getAsFileSystemHandle?.() ?? Promise.resolve(null); | ||
| } catch { | ||
| return Promise.resolve(null); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Log the dropped-handle failure instead of swallowing it.
Line 139 silently discards getAsFileSystemHandle() errors, so drag/drop can lose refreshability with no signal about why. Please log or rethrow here instead of using an empty catch.
♻️ Proposed fix
try {
return item.getAsFileSystemHandle?.() ?? Promise.resolve(null);
- } catch {
+ } catch (err) {
+ console.warn('[file-system-access] getAsFileSystemHandle failed', err);
return Promise.resolve(null);
}As per coding guidelines, "No silent catch {} — log or rethrow errors".
📝 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.
Suggested change
| try { | |
| return item.getAsFileSystemHandle?.() ?? Promise.resolve(null); | |
| } catch { | |
| return Promise.resolve(null); | |
| try { | |
| return item.getAsFileSystemHandle?.() ?? Promise.resolve(null); | |
| } catch (err) { | |
| console.warn('[file-system-access] getAsFileSystemHandle failed', err); | |
| return Promise.resolve(null); | |
| } |
🤖 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 `@apps/viewer/src/services/file-system-access.ts` around lines 137 - 140, The
dropped-handle fallback in getAsFileSystemHandle is silently swallowing
exceptions, which hides drag/drop failures. Update the try/catch in
file-system-access.ts so the catch block either logs the caught error with
enough context or rethrows it instead of returning a silent null, and keep the
existing getAsFileSystemHandle() path behavior unchanged when it succeeds.
Source: Coding guidelines
Comment on lines
+339
to
+344
| * Live File System Access handle captured when the model was opened via | ||
| * `showOpenFilePicker` (Chromium only). Unlike `sourceFile` — a frozen | ||
| * snapshot of the bytes at pick time — this can be re-read with `getFile()` | ||
| * to pull the current on-disk contents, powering the "Refresh" action | ||
| * (issue #1345). Absent for drag-drop, `<input type="file">`, cache-restored, | ||
| * and federated loads. Held in memory only; never serialized to cache. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the sourceHandle lifecycle comment.
Line 343 says this field is absent for federated loads, but useIfcFederation.loadFilesSequentially() now threads handles into federated models so the toolbar can refresh them. That comment should only exclude federated loads that were created without live handles.
♻️ Proposed fix
- * (issue `#1345`). Absent for drag-drop, `<input type="file">`, cache-restored,
- * and federated loads. Held in memory only; never serialized to cache.
+ * (issue `#1345`). Absent for `<input type="file">`, cache-restored, IFCX
+ * composition, and any load path that did not originate from a live FS Access
+ * handle. Held in memory only; never serialized to cache.📝 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.
Suggested change
| * Live File System Access handle captured when the model was opened via | |
| * `showOpenFilePicker` (Chromium only). Unlike `sourceFile` — a frozen | |
| * snapshot of the bytes at pick time — this can be re-read with `getFile()` | |
| * to pull the current on-disk contents, powering the "Refresh" action | |
| * (issue #1345). Absent for drag-drop, `<input type="file">`, cache-restored, | |
| * and federated loads. Held in memory only; never serialized to cache. | |
| * Live File System Access handle captured when the model was opened via | |
| * `showOpenFilePicker` (Chromium only). Unlike `sourceFile` — a frozen | |
| * snapshot of the bytes at pick time — this can be re-read with `getFile()` | |
| * to pull the current on-disk contents, powering the "Refresh" action | |
| * (issue `#1345`). Absent for `<input type="file">`, cache-restored, IFCX | |
| * composition, and any load path that did not originate from a live FS Access | |
| * handle. Held in memory only; never serialized to cache. |
🤖 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 `@apps/viewer/src/store/types.ts` around lines 339 - 344, Update the
`sourceHandle` lifecycle documentation in `types.ts` so it matches the new
federation behavior: `useIfcFederation.loadFilesSequentially()` can now attach
live handles to federated models for refresh support. Adjust the comment on
`sourceHandle` to exclude only non-handle federated loads, while still noting it
is absent for drag-drop, `<input type="file">`, cache-restored, and any
federated load created without a live handle.
… txn) cacheFileBlobs awaited file.arrayBuffer() between opening the IndexedDB transaction and the store.put(). An IndexedDB transaction auto-commits the moment control returns to the event loop with no pending request, so awaiting a non-IDB promise inactivated it and the put() threw TransactionInactiveError, which the empty catch swallowed. Nothing was ever cached, so every command palette recent file missed the cache and fell back to re-picking (the file dialog the user kept seeing). Read all blobs up front, then write them in one synchronous burst inside the transaction, and log instead of silently swallowing failures. Recent files now load straight from cache; the picker fallback only fires for genuinely uncached entries (evicted, oversized, or first seen before this fix).
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)
apps/viewer/src/lib/recent-files.ts (1)
108-132: 🩺 Stability & Availability | 🟠 MajorClose the IndexedDB connection in
finally.db.close()only runs on the success path; any failure afteropenDB()leaves the connection open. Wrap the transaction block intry/finallyand close the DB there.🤖 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 `@apps/viewer/src/lib/recent-files.ts` around lines 108 - 132, The IndexedDB connection is only being closed on the success path in the recent-files persistence flow. Update the logic in the openDB/transaction block inside the recent-files helper so that db.close() always runs in a finally clause, even if the transaction fails or an exception is thrown after opening the database. Keep the transaction and eviction logic in the existing helper, but wrap it so the connection cleanup is guaranteed regardless of outcome.
🤖 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 `@apps/viewer/src/lib/recent-files.ts`:
- Around line 95-105: `cacheFileBlobs()` is still loading every cacheable file
into memory before the transaction, which can exceed the intended cache
capacity; limit the staged reads to the cache size up front. Update the loop
that builds `records` so it only processes at most the first 5 eligible files
before calling `file.arrayBuffer()`, using the existing `MAX_CACHE_SIZE`/cache
logic in `cacheFileBlobs()` to skip oversized files and stop once the cache
limit is reached.
---
Outside diff comments:
In `@apps/viewer/src/lib/recent-files.ts`:
- Around line 108-132: The IndexedDB connection is only being closed on the
success path in the recent-files persistence flow. Update the logic in the
openDB/transaction block inside the recent-files helper so that db.close()
always runs in a finally clause, even if the transaction fails or an exception
is thrown after opening the database. Keep the transaction and eviction logic in
the existing helper, but wrap it so the connection cleanup is guaranteed
regardless of outcome.
🪄 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: efd8ec61-bfb6-42ac-9bbc-5eaa7e883771
📒 Files selected for processing (1)
apps/viewer/src/lib/recent-files.ts
Comment on lines
+95
to
+105
| const records: { name: string; blob: ArrayBuffer; size: number; type: string; timestamp: number }[] = []; | ||
| for (const file of files) { | ||
| if (file.size > MAX_CACHE_SIZE) continue; // skip oversized files | ||
| records.push({ | ||
| name: file.name, | ||
| blob: await file.arrayBuffer(), | ||
| size: file.size, | ||
| type: file.type, | ||
| timestamp: Date.now(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and inspect its structure before reading relevant sections.
git ls-files 'apps/viewer/src/lib/recent-files.ts'
wc -l apps/viewer/src/lib/recent-files.ts
ast-grep outline apps/viewer/src/lib/recent-files.ts --view expanded
sed -n '1,220p' apps/viewer/src/lib/recent-files.tsRepository: LTplus-AG/ifc-lite
Length of output: 7300
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect references to the cache constants and the save path to understand actual behavior.
rg -n "MAX_CACHE_SIZE|MAX_CACHED_FILES|recent-files|arrayBuffer\\(" apps/viewer/src/lib apps/viewer/src -g '!**/*.map'Repository: LTplus-AG/ifc-lite
Length of output: 5736
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the call sites to understand how many files can reach cacheFileBlobs().
sed -n '1,220p' apps/viewer/src/components/viewer/MainToolbar.tsx
printf '\n---\n'
sed -n '520,660p' apps/viewer/src/components/viewer/ViewportContainer.tsx
printf '\n---\n'
sed -n '1,180p' apps/viewer/src/components/viewer/MobileToolbar.tsxRepository: LTplus-AG/ifc-lite
Length of output: 20018
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the file open flow to see whether cacheFileBlobs() receives small batches or arbitrary multi-file selections.
ast-grep outline apps/viewer/src/components/viewer/MainToolbar.tsx --view expanded
ast-grep outline apps/viewer/src/components/viewer/ViewportContainer.tsx --view expanded
ast-grep outline apps/viewer/src/components/viewer/MobileToolbar.tsx --view expandedRepository: LTplus-AG/ifc-lite
Length of output: 2110
Bound staged blob reads to the cache capacity.
cacheFileBlobs() still reads every cacheable file with file.arrayBuffer() before the transaction, and multi-file opens can pass many files. That can spike peak memory far beyond the 5 entries you keep. Slice to the cache limit before reading bytes.
🤖 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 `@apps/viewer/src/lib/recent-files.ts` around lines 95 - 105,
`cacheFileBlobs()` is still loading every cacheable file into memory before the
transaction, which can exceed the intended cache capacity; limit the staged
reads to the cache size up front. Update the loop that builds `records` so it
only processes at most the first 5 eligible files before calling
`file.arrayBuffer()`, using the existing `MAX_CACHE_SIZE`/cache logic in
`cacheFileBlobs()` to skip oversized files and stop once the cache limit is
reached.
…1345) - Case-insensitive IFCX routing: add isIfcxModelFile so MODEL.IFCX takes the IFCX composition/overlay path (isSupportedModelFile already lowercases). - Filter the picker's "all files" results in MainToolbar.handleOpenClick before loading, matching the <input> and Add Model paths. - Await the single-model reload so Refresh reports success only after it lands. - aria-label on the icon-only Refresh button. - Drop handling: pair each file with its handle from the same dropped item instead of matching by filename (handles same-name collisions). - Log getAsFileSystemHandle failures instead of swallowing them. - Bound cacheFileBlobs staged reads to the cache capacity (MAX_CACHED_FILES). - Correct the FederatedModel.sourceHandle lifecycle comment (drag-drop and federated loads now capture handles; <input>/cache/IFCX-layers do not).
Collaborator
Author
|
Addressed the CodeRabbit review in
Also fixed (separate finding, surfaced while testing on Chrome): |
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)
apps/viewer/src/components/viewer/ViewportContainer.tsx (1)
1131-1137: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t open the FS picker after an awaited cache miss.
Line 1137 calls
handleOpenClick()afterawait getCachedFile(file). In Chromium,showOpenFilePickerneeds transient user activation, which may be gone after the async IndexedDB lookup, so stale/uncached recent entries can fail to reopen. Preload cached names and decide synchronously whether to callhandleOpenClick(), or show a “use Open” prompt after an async miss.🤖 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 `@apps/viewer/src/components/viewer/ViewportContainer.tsx` around lines 1131 - 1137, The onClick flow in ViewportContainer currently awaits getCachedFile(file) before calling handleOpenClick(), which can lose transient user activation before the file picker opens. Update the recent-entry/open logic so the decision to invoke handleOpenClick() is made synchronously from preloaded cache metadata, or otherwise surface an explicit “use Open” prompt after an async miss instead of opening the picker after the await. Keep the fix localized around getCachedFile, loadFile, and handleOpenClick in ViewportContainer.
🤖 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 `@apps/viewer/src/components/viewer/ViewportContainer.tsx`:
- Around line 505-518: In ViewportContainer’s drag-drop handling, the current
handlesPromise.then logic treats any captured handle set as authoritative and
can ignore other supported dropped files when handle capture is partial or
rejected. Update the handling around supportedOpened, useHandles, and routeLoad
so you only prefer paired handles when all supported files were captured;
otherwise fall back to the plain supportedFiles list and ensure a rejected
handlesPromise still routes to the dropped files.
---
Outside diff comments:
In `@apps/viewer/src/components/viewer/ViewportContainer.tsx`:
- Around line 1131-1137: The onClick flow in ViewportContainer currently awaits
getCachedFile(file) before calling handleOpenClick(), which can lose transient
user activation before the file picker opens. Update the recent-entry/open logic
so the decision to invoke handleOpenClick() is made synchronously from preloaded
cache metadata, or otherwise surface an explicit “use Open” prompt after an
async miss instead of opening the picker after the await. Keep the fix localized
around getCachedFile, loadFile, and handleOpenClick in ViewportContainer.
🪄 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: ffad5955-721d-4252-b427-79628fbc78fa
📒 Files selected for processing (5)
apps/viewer/src/components/viewer/MainToolbar.tsxapps/viewer/src/components/viewer/ViewportContainer.tsxapps/viewer/src/lib/recent-files.tsapps/viewer/src/services/file-system-access.tsapps/viewer/src/store/types.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/viewer/src/store/types.ts
- apps/viewer/src/lib/recent-files.ts
- apps/viewer/src/services/file-system-access.ts
- apps/viewer/src/components/viewer/MainToolbar.tsx
Comment on lines
+505
to
+518
| void handlesPromise.then((opened) => { | ||
| // Prefer the handle-paired files (Chromium): each file + handle comes from | ||
| // the same dropped item, so no filename matching is needed. Fall back to | ||
| // the plain dropped files when no handles were captured (Firefox/Safari). | ||
| const supportedOpened = (opened ?? []).filter((o) => isSupportedFile(o.file)); | ||
| const useHandles = supportedOpened.length > 0; | ||
| const files = useHandles ? supportedOpened.map((o) => o.file) : supportedFiles; | ||
| const handles = useHandles ? supportedOpened.map((o) => o.handle) : undefined; | ||
|
|
||
| if (hasModelsLoaded) { | ||
| // Models already loaded - add new files sequentially | ||
| loadFilesSequentially(supportedFiles); | ||
| } else if (supportedFiles.length === 1) { | ||
| // Single file, no models loaded - use loadFile | ||
| loadFile(supportedFiles[0]); | ||
| } else { | ||
| // Multiple files, no models loaded - use federation | ||
| resetViewerState(); | ||
| clearAllModels(); | ||
| loadFilesSequentially(supportedFiles); | ||
| } | ||
| }, [loadFile, loadFilesSequentially, resetViewerState, clearAllModels, webgpu.supported, hasModelsLoaded]); | ||
| recordRecentFiles(files.map((file) => ({ name: file.name, size: file.size }))); | ||
| void cacheFileBlobs(files); | ||
| setRecentFiles(getRecentFiles().slice(0, 3)); | ||
|
|
||
| routeLoad(files, handles); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fall back when drag handle capture is partial or rejected.
Line 510 treats any captured handle as authoritative, so a partial Chromium handle capture loads only that subset and silently drops the remaining supported files. Also add a rejection fallback so failed handle capture still loads the dropped files.
Proposed fix
- void handlesPromise.then((opened) => {
+ void handlesPromise.then((opened) => {
// Prefer the handle-paired files (Chromium): each file + handle comes from
// the same dropped item, so no filename matching is needed. Fall back to
// the plain dropped files when no handles were captured (Firefox/Safari).
const supportedOpened = (opened ?? []).filter((o) => isSupportedFile(o.file));
- const useHandles = supportedOpened.length > 0;
+ const useHandles = supportedOpened.length === supportedFiles.length;
const files = useHandles ? supportedOpened.map((o) => o.file) : supportedFiles;
const handles = useHandles ? supportedOpened.map((o) => o.handle) : undefined;
recordRecentFiles(files.map((file) => ({ name: file.name, size: file.size })));
void cacheFileBlobs(files);
setRecentFiles(getRecentFiles().slice(0, 3));
routeLoad(files, handles);
- });
+ }).catch((err) => {
+ console.warn('[file-system-access] dropped file handle capture failed; loading files without handles', err);
+ recordRecentFiles(supportedFiles.map((file) => ({ name: file.name, size: file.size })));
+ void cacheFileBlobs(supportedFiles);
+ setRecentFiles(getRecentFiles().slice(0, 3));
+ routeLoad(supportedFiles);
+ });🤖 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 `@apps/viewer/src/components/viewer/ViewportContainer.tsx` around lines 505 -
518, In ViewportContainer’s drag-drop handling, the current handlesPromise.then
logic treats any captured handle set as authoritative and can ignore other
supported dropped files when handle capture is partial or rejected. Update the
handling around supportedOpened, useHandles, and routeLoad so you only prefer
paired handles when all supported files were captured; otherwise fall back to
the plain supportedFiles list and ensure a rejected handlesPromise still routes
to the dropped files.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1345. Adds a Refresh button to reload the open model without closing and re-opening the same file, so it can be monitored during design.
The existing Open flow uses
<input type="file">, which hands back a File that is a frozen snapshot of the bytes at pick time. Re-reading it can never reflect on-disk edits. Only the File System Access API (showOpenFilePicker→FileSystemFileHandle.getFile()) returns a fresh snapshot of the current file. So the Open button now prefers that API when available (Chromium / secure context) to capture a live handle, and falls back to<input type="file">everywhere else.Changes (
apps/viewer)services/file-system-access.ts(new) —supportsFileSystemAccess(),openIfcFilesWithHandles()(picks files + captures live handles; user-cancel handled),readFreshFile(handle)(permission-checked re-read of current on-disk bytes; surfaces moved/deleted/denied).types/file-system-access.d.ts(new) — minimal ambient declarations for the API surface missing from TS 6.0.3lib.dom(Window.showOpenFilePicker,OpenFilePickerOptions,queryPermission/requestPermission).FileSystemFileHandle.getFile()already ships in lib.dom.store/types.ts—FederatedModel.sourceHandle?: FileSystemFileHandle(in-memory only, never serialized to cache; mirrorssourceFile).hooks/useIfcLoader.ts—loadFileaccepts an optional{ sourceHandle }and persists it on the primary model record.components/viewer/MainToolbar.tsx— Open button prefers the FS Access API; new Refresh button (RefreshCw).Behaviour / scope
<input type="file">, recent-files-from-cache, and federated multi-model loads have no handle, so the button isn't offered (no dead/disabled control).<input type="file">transparently — no Refresh button, no regression.Testing
tsc --noEmitclean for all touched files (remaining errors are pre-existing un-built-package gaps in the worktree, unrelated to this change).Follow-up (not in this PR)
DataTransferItem.getAsFileSystemHandle(), extending Refresh to dropped files.🤖 Generated with Claude Code
Summary by CodeRabbit