feat: add select-all controls to property panel #175
Conversation
WalkthroughIntroduces a selection index service and integrates it into IFC context and the ModelInfo UI. Adds background per-model indexing, readiness tracking, and new APIs to select elements by index or property. Updates ModelInfo to provide “Select All” actions per row. Adds a new i18n key across four locales. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as ModelInfo (UI)
participant Ctx as IFCContext
participant Idx as SelectionIndex
participant API as IfcAPI
participant Sel as Selection Manager
Note over Ctx,Idx: Background on model load
Ctx->>Idx: isReady(API, modelID)?
alt not ready
Ctx->>Idx: build(API, modelID, onProgress)
Idx-->>Ctx: progress updates / ready
Ctx->>Ctx: selectionIndexReadyByModel[modelID]=true
end
Note over UI: User clicks "Select All"
UI->>UI: toIndexKey(path,value)
alt key supported AND ready
UI->>Ctx: selectElementsByIndex(modelID, key, value)
Ctx->>Idx: query(API, modelID, key, value)
Idx-->>Ctx: [{modelID, expressID}]*
Ctx->>Sel: select elements by IDs
Sel-->>UI: selection updated
else unsupported or not ready
UI->>Ctx: selectElementsByProperty(modelID, path, value)
Ctx->>API: batched property lookups (yielding)
Ctx-->>UI: selection updated
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Added support for indexed selection in the IFC context, allowing for faster element selection based on predefined keys (IFC_CLASS, NAME, TYPE_NAME, PSET_COMMON). - Introduced a readiness flag for selection indices to ensure efficient selection operations. - Updated the ModelInfo component to utilize the new selection index, enhancing the user experience with immediate feedback on selection actions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/model-info.tsx (2)
90-99: Don’t join React nodes; render with separators
map(...).join(", ")coerces React nodes to strings. Render with fragments and separators.- const displayValues = value.values - .map((v: any) => renderPropertyValue(v, keyHint, ifcApi, t)) // Recursively render each value in the array - .join(", "); - return ( - <> - {displayValues}{" "} - <span className="text-muted-foreground/80">({value.unit})</span> - </> - ); + return ( + <> + {value.values.map((v: any, i: number) => ( + <React.Fragment key={i}> + {renderPropertyValue(v, keyHint, ifcApi, t)} + {i < value.values.length - 1 ? ", " : ""} + </React.Fragment> + ))}{" "} + <span className="text-muted-foreground/80">({value.unit})</span> + </> + );
174-192: Same issue for plain arrays: avoid.joinon React nodesUse fragments with separators.
- if ( - value.length <= 5 && // Allow slightly longer lists if they are simple - value.every( - (v) => - typeof v === "string" || - typeof v === "number" || - typeof v === "boolean", - ) - ) { - return value.map((v) => renderPropertyValue(v, keyHint, ifcApi, t)).join(", "); // Render each item - } + if ( + value.length <= 5 && + value.every((v) => typeof v === "string" || typeof v === "number" || typeof v === "boolean") + ) { + return ( + <> + {value.map((v, i) => ( + <React.Fragment key={i}> + {renderPropertyValue(v, keyHint, ifcApi, t)} + {i < value.length - 1 ? ", " : ""} + </React.Fragment> + ))} + </> + ); + }Alternatively, extract a small
joinNodeshelper for reuse.
🧹 Nitpick comments (10)
services/selection-index.ts (6)
59-63: Make unwrap recursive for nested IFC value wrappers.Occasionally values are nested (e.g.,
{ value: { value: "X" } }). Recursive unwrap avoids silent mismatches.Apply this diff:
-function unwrap(val: any): any { - if (val && typeof val === "object" && "value" in val) return val.value; - return val; -} +function unwrap(val: any): any { + let v = val; + while (v && typeof v === "object" && "value" in v) v = (v as any).value; + return v; +}
179-214: Avoid unnecessary awaits on synchronous IfcAPI calls.
GetLine,GetAllLines, andGetMaxExpressIDare sync in web-ifc. Droppingawaitin hot loops reduces microtask overhead.Example minimal change:
-const line = await ifcApi.GetLine(modelID, expressID, false); +const line = ifcApi.GetLine(modelID, expressID, false); ... -const maxId = ifcApi.GetMaxExpressID(modelID); +const maxId = ifcApi.GetMaxExpressID(modelID);Also applies to: 215-244
275-337: Reduce duplicate type lookups (Step 4 vs 4b).You fetch each type line twice. Cache
typeObjor merge into one pass to cut API calls ~50% on large models.Also applies to: 339-391
516-531: Expose richer stats hooks (optional).Consider counts per bucket kind and a memory estimate to support UI readiness/debug.
151-166: Cancellation hook (optional).If a model unloads mid-build, add an abort signal to stop indexing work early.
4-7: Import shared SelectedElementInfo instead of redefining (services/selection-index.ts:4-7)
services/selection-index.ts duplicates theSelectedElementInfointerface fromcontext/ifc-context.tsx. Extract it into a common module or import the existing definition to prevent drift.context/ifc-context.tsx (1)
1777-1884: Property-based selection: consider safer equality and fewer allocations
valuesEqualviaJSON.stringifyis order- and type-fragile and can be slow. Normalize scalars and unwrap{ value }first, then compare strings case-insensitively where appropriate, numbers viaNumber.isFinite, and booleans explicitly.- For the attribute fast-path, batching
GetLinecalls is good; consider pre-checking trivial mismatches to skip JSON stringify entirely.components/model-info.tsx (3)
604-617: Broaden index-key mapping to cover Type PSet_CommonType-derived sets often render as “Pset_*Common (from Type: …)”. Normalize the name so these can use the indexed PSET_COMMON bucket instead of falling back.
- if (path.length >= 3 && path[0] === "propertySets" && /^PSet_.*Common$/i.test(path[1])) { - const psetName = path[1]; - const propName = path[2]; - return { kind: "PSET_COMMON", psetName, propName }; - } + if (path.length >= 3 && (path[0] === "propertySets" || path[0] === "typeSets")) { + const psetNameNorm = path[1].replace(/\s*\(from Type:.*\)\s*$/i, ""); + if (!/^Pset_.*Common$/i.test(psetNameNorm)) return null; + const propName = path[2]; + return { kind: "PSET_COMMON", psetName: psetNameNorm, propName }; + }
649-672: Minor UX: avoid nested TooltipProviders and stop event bubblingThere’s a
TooltipProviderwrapping anotherTooltipProvider. Also, you manually stop propagation here; consider doing it in the shared handler for consistency.
769-777: After adding the fallback, these buttons will work across all rowsOnce
createSelectAllHandlerfalls back toselectElementsByProperty, Select All will function for Express ID, Model, GlobalId, generic attributes, and all (type/instance) property sets. Consider showing a subtle loading state when falling back (since it yields in chunks).Also applies to: 785-789, 798-802, 884-889, 924-929
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
components/model-info.tsx(15 hunks)context/ifc-context.tsx(9 hunks)public/locales/de/common.json(1 hunks)public/locales/en/common.json(1 hunks)public/locales/fr/common.json(1 hunks)public/locales/it/common.json(1 hunks)services/selection-index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/posthog-integration.mdc)
**/*.{ts,tsx,js,jsx}: If using TypeScript, use an enum to store flag names. If using JavaScript, store flag names as strings to an object declared as a constant, to simulate an enum. Use a consistent naming convention for this storage. enum/const object members should be written UPPERCASE_WITH_UNDERSCORE.
Gate flag-dependent code on a check that verifies the flag's values are valid and expected.
If a custom property for a person or event is at any point referenced in two or more files or two or more callsites in the same file, use an enum or const object, as above in feature flags.
Files:
services/selection-index.tscomponents/model-info.tsxcontext/ifc-context.tsx
🧬 Code graph analysis (3)
services/selection-index.ts (1)
context/ifc-context.tsx (1)
SelectedElementInfo(75-78)
components/model-info.tsx (2)
components/ui/tooltip.tsx (4)
TooltipProvider(30-30)Tooltip(30-30)TooltipTrigger(30-30)TooltipContent(30-30)services/selection-index.ts (1)
IndexKey(15-19)
context/ifc-context.tsx (2)
services/selection-index.ts (3)
IndexKey(15-19)SelectionIndex(151-531)SelectedElementInfo(4-7)services/ifc-element-extractor.ts (1)
IFCElementExtractor(12-417)
🔇 Additional comments (6)
public/locales/de/common.json (1)
26-26: LGTM — new key added correctly.Translation and placement are consistent with other locales.
public/locales/en/common.json (1)
26-26: LGTM — new key added correctly.Matches feature intent; no typos.
public/locales/it/common.json (1)
26-26: LGTM — new key added correctly.Translation reads naturally.
public/locales/fr/common.json (1)
26-26: LGTM — new key added correctly.Consistent with phrasing used elsewhere (e.g., “Sélectionner tout le visible”).
context/ifc-context.tsx (2)
1763-1775: Indexed selection: LGTMGuard + query + delegate to
selectElementslooks correct.
2474-2478: API exposure: LGTMNew functions and readiness flag are correctly exposed on the context value.
| const createSelectAllHandler = (path: string[], value: any) => () => { | ||
| const key = toIndexKey(path, value); | ||
| if (key) { | ||
| if (selectionIndexReadyByModel[modelID]) { | ||
| selectElementsByIndex(modelID, key, value); | ||
| } else { | ||
| console.warn("Selection index not ready; ignoring Select All to avoid slow path."); | ||
| } | ||
| return; | ||
| } | ||
| console.warn("Select All unsupported for path", path.join(".")); | ||
| }; | ||
|
|
There was a problem hiding this comment.
“Select All” is a no-op for most rows; add property-based fallback
Currently only IFC Class, Name, and some PSet_Common paths work. For Description, ObjectType, GlobalId, Express ID, Model, generic attributes, and most set properties, toIndexKey returns null and the handler only logs a warning. Wire up selectElementsByProperty as a fallback (and when the index isn’t ready).
-const createSelectAllHandler = (path: string[], value: any) => () => {
+const createSelectAllHandler = (path: string[], value: any) => async () => {
const key = toIndexKey(path, value);
if (key) {
- if (selectionIndexReadyByModel[modelID]) {
- selectElementsByIndex(modelID, key, value);
- } else {
- console.warn("Selection index not ready; ignoring Select All to avoid slow path.");
- }
- return;
+ if (selectionIndexReadyByModel[modelID]) {
+ selectElementsByIndex(modelID, key, value);
+ } else {
+ // Fallback to property-based search until index is ready
+ await selectElementsByProperty(modelID, path, value);
+ }
+ return;
}
- console.warn("Select All unsupported for path", path.join("."));
+ // Fallback for unsupported keys
+ await selectElementsByProperty(modelID, path, value);
};📝 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.
| const createSelectAllHandler = (path: string[], value: any) => () => { | |
| const key = toIndexKey(path, value); | |
| if (key) { | |
| if (selectionIndexReadyByModel[modelID]) { | |
| selectElementsByIndex(modelID, key, value); | |
| } else { | |
| console.warn("Selection index not ready; ignoring Select All to avoid slow path."); | |
| } | |
| return; | |
| } | |
| console.warn("Select All unsupported for path", path.join(".")); | |
| }; | |
| const createSelectAllHandler = (path: string[], value: any) => async () => { | |
| const key = toIndexKey(path, value); | |
| if (key) { | |
| if (selectionIndexReadyByModel[modelID]) { | |
| selectElementsByIndex(modelID, key, value); | |
| } else { | |
| // Fallback to property-based search until index is ready | |
| await selectElementsByProperty(modelID, path, value); | |
| } | |
| return; | |
| } | |
| // Fallback for unsupported keys | |
| await selectElementsByProperty(modelID, path, value); | |
| }; |
🤖 Prompt for AI Agents
In components/model-info.tsx around lines 619 to 631, the current
createSelectAllHandler only calls selectElementsByIndex when toIndexKey(path,
value) returns a key and the selection index is ready, otherwise it logs a
warning and becomes a no-op; change it to fall back to selectElementsByProperty
whenever toIndexKey returns null or when the index isn't ready (i.e., call
selectElementsByProperty(modelID, path, value) in those branches), preserving
the existing selectElementsByIndex call for the fast path and keeping console
warnings for visibility.
| IFCElementExtractor.clearCache(); | ||
| PropertyCache.clearCache(); | ||
| elementPropsCache.current.clear(); | ||
| try { SelectionIndex.clear(); } catch { } | ||
| setLoadedModels([commonLoadLogic(url, name, fileId)]); | ||
| return null; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reset readiness map on model replacement
When replacing a model you clear SelectionIndex, element caches, and loaded models, but you don't reset selectionIndexReadyByModel. This can leave stale ready flags.
IFCElementExtractor.clearCache();
PropertyCache.clearCache();
elementPropsCache.current.clear();
try { SelectionIndex.clear(); } catch { }
+ setSelectionIndexReadyByModel({});
setLoadedModels([commonLoadLogic(url, name, fileId)]);📝 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.
| IFCElementExtractor.clearCache(); | |
| PropertyCache.clearCache(); | |
| elementPropsCache.current.clear(); | |
| try { SelectionIndex.clear(); } catch { } | |
| setLoadedModels([commonLoadLogic(url, name, fileId)]); | |
| return null; | |
| }, | |
| IFCElementExtractor.clearCache(); | |
| PropertyCache.clearCache(); | |
| elementPropsCache.current.clear(); | |
| try { SelectionIndex.clear(); } catch { } | |
| setSelectionIndexReadyByModel({}); | |
| setLoadedModels([commonLoadLogic(url, name, fileId)]); | |
| return null; |
🤖 Prompt for AI Agents
In context/ifc-context.tsx around lines 1437 to 1443, when replacing a model you
clear SelectionIndex, element caches, and loaded models but forget to reset
selectionIndexReadyByModel; update this block to also clear or reinitialize the
selectionIndexReadyByModel map/record (e.g., set it to an empty object or call
its clear method) so any stale ready flags are removed when a new model is
loaded.
| // Clear selection index for this model | ||
| try { SelectionIndex.clear(ifcApiInternal, m.modelID); } catch { } | ||
| } catch (e) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clear per-model property caches and tighten readiness removal
On removal, you clear the SelectionIndex, but retained per-model caches may leak memory. Also, compute the modelID to drop before mutating state to avoid stale captures.
- // Clear selection index for this model
- try { SelectionIndex.clear(ifcApiInternal, m.modelID); } catch { }
+ // Clear selection index and caches for this model
+ try { SelectionIndex.clear(ifcApiInternal, m.modelID); } catch { }
+ try { PropertyCache.clearCache(m.modelID); } catch { }
+ elementPropsCache.current.delete(m.modelID);Optionally, move the readiness cleanup to use the modelID you already have in-scope:
- setSelectionIndexReadyByModel(prev => {
- const next = { ...prev } as Record<number, boolean>;
- const removedModel = loadedModels.find((m) => m.id === id);
- if (removedModel?.modelID != null) delete next[removedModel.modelID];
- return next;
- });
+ const removed = loadedModels.find((m) => m.id === id);
+ if (removed?.modelID != null) {
+ setSelectionIndexReadyByModel(prev => {
+ const next = { ...prev };
+ delete next[removed.modelID!];
+ return next;
+ });
+ }Also applies to: 1482-1488
🤖 Prompt for AI Agents
In context/ifc-context.tsx around lines 1456-1458, the code clears
SelectionIndex inside a try/catch after state mutations which can retain
per-model caches and capture a stale modelID; compute and store the modelID
before mutating state, call SelectionIndex.clear(ifcApiInternal, modelID) and
also clear any other per-model caches (e.g., geometry, property, or spatial
caches) using that modelID, and use that same modelID when removing readiness
entries so the cleanup targets the correct model; repeat the same changes for
the analogous block at lines 1482-1488.
| for (const m of modelsWithId) { | ||
| const mid = m.modelID as number; | ||
| if (selectionIndexReadyByModel[mid]) continue; | ||
| try { | ||
| bufferedConsole.current.updateProgress(`Indexing model ${mid}…`, 1); | ||
| await SelectionIndex.build(ifcApiInternal, mid, (percent, message) => { | ||
| bufferedConsole.current.updateProgress(message || `Indexing…`, Math.max(1, Math.min(90, percent))); | ||
| }); | ||
| setSelectionIndexReadyByModel(prev => ({ ...prev, [mid]: true })); | ||
| bufferedConsole.current.updateProgress(`Index ready`, 92); | ||
| } catch (e) { | ||
| console.warn(`Selection index build failed for model ${mid}`, e); | ||
| } | ||
| } | ||
| }; | ||
| buildIndices(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [ifcApiInternal, loadedModels.map(m => m.modelID).join(',')]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid duplicate index builds and decouple from rule progress UI
The effect can launch concurrent builds if the deps change before setSelectionIndexReadyByModel[mid] flips to true. Also, publishing index progress through the shared bufferedConsole drives the rule-progress UI, which is misleading.
- Add a guard that treats already-built indices as ready (via
SelectionIndex.isReady) to prevent re-building. - Consider using a separate progress channel (or no console updates) for indexing to avoid interfering with rule progress.
Apply this minimal guard diff:
for (const m of modelsWithId) {
const mid = m.modelID as number;
- if (selectionIndexReadyByModel[mid]) continue;
+ if (selectionIndexReadyByModel[mid]) continue;
+ // If an index was already built elsewhere (e.g., prior run), mark ready and skip.
+ if (SelectionIndex.isReady(ifcApiInternal, mid)) {
+ setSelectionIndexReadyByModel(prev => ({ ...prev, [mid]: true }));
+ continue;
+ }
try {
bufferedConsole.current.updateProgress(`Indexing model ${mid}…`, 1);📝 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.
| for (const m of modelsWithId) { | |
| const mid = m.modelID as number; | |
| if (selectionIndexReadyByModel[mid]) continue; | |
| try { | |
| bufferedConsole.current.updateProgress(`Indexing model ${mid}…`, 1); | |
| await SelectionIndex.build(ifcApiInternal, mid, (percent, message) => { | |
| bufferedConsole.current.updateProgress(message || `Indexing…`, Math.max(1, Math.min(90, percent))); | |
| }); | |
| setSelectionIndexReadyByModel(prev => ({ ...prev, [mid]: true })); | |
| bufferedConsole.current.updateProgress(`Index ready`, 92); | |
| } catch (e) { | |
| console.warn(`Selection index build failed for model ${mid}`, e); | |
| } | |
| } | |
| }; | |
| buildIndices(); | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [ifcApiInternal, loadedModels.map(m => m.modelID).join(',')]); | |
| for (const m of modelsWithId) { | |
| const mid = m.modelID as number; | |
| if (selectionIndexReadyByModel[mid]) continue; | |
| // If an index was already built elsewhere (e.g., prior run), mark ready and skip. | |
| if (SelectionIndex.isReady(ifcApiInternal, mid)) { | |
| setSelectionIndexReadyByModel(prev => ({ ...prev, [mid]: true })); | |
| continue; | |
| } | |
| try { | |
| bufferedConsole.current.updateProgress(`Indexing model ${mid}…`, 1); | |
| await SelectionIndex.build(ifcApiInternal, mid, (percent, message) => { | |
| bufferedConsole.current.updateProgress( | |
| message || `Indexing…`, | |
| Math.max(1, Math.min(90, percent)) | |
| ); | |
| }); | |
| setSelectionIndexReadyByModel(prev => ({ ...prev, [mid]: true })); | |
| bufferedConsole.current.updateProgress(`Index ready`, 92); | |
| } catch (e) { | |
| console.warn(`Selection index build failed for model ${mid}`, e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In context/ifc-context.tsx around lines 1537 to 1555, the effect can re-launch
concurrent selection-index builds and pollute the rule-progress UI; add a guard
that treats indices already built as ready by calling
SelectionIndex.isReady(mid) (or equivalent) before queuing a build and skip
building if true, and stop using the shared
bufferedConsole.current.updateProgress for indexing (use a separate progress
channel or omit console updates) so index progress does not drive the
rule-progress UI; after successful build call setSelectionIndexReadyByModel(prev
=> ({ ...prev, [mid]: true })) as before and keep the try/catch behavior.
| @@ -0,0 +1,533 @@ | |||
| import type { IfcAPI } from "web-ifc"; | |||
There was a problem hiding this comment.
Fix TS type errors: IfcAPI doesn’t declare .properties; widen the type and update method sigs.
Current usages of ifcApi.properties.getTypeProperties/getPropertySets will fail type-checking. Define an augmented type and use it in the public API.
Apply this diff:
import type { IfcAPI } from "web-ifc";
+// Augment IfcAPI to support optional properties utils present in some runtimes (e.g., IFC.js bindings)
+type IfcApiWithProps = IfcAPI & {
+ properties?: {
+ getTypeProperties?: (modelID: number, expressID: number, includeAll?: boolean) => Promise<any[]>;
+ getPropertySets?: (
+ modelID: number,
+ expressID: number,
+ includeInherited?: boolean,
+ includeQuantities?: boolean
+ ) => Promise<any[]>;
+ };
+};
export const SelectionIndex = {
- isReady(ifcApi: IfcAPI, modelID: number): boolean {
+ isReady(ifcApi: IfcApiWithProps, modelID: number): boolean {
...
- async build(
- ifcApi: IfcAPI,
+ async build(
+ ifcApi: IfcApiWithProps,
modelID: number,
onProgress?: (percent: number, message?: string) => void,
): Promise<void> {
...
- query(
- ifcApi: IfcAPI,
+ query(
+ ifcApi: IfcApiWithProps,
modelID: number,
key: IndexKey,
value: unknown,
): SelectedElementInfo[] {
...
- clear(ifcApi?: IfcAPI, modelID?: number): void {
+ clear(ifcApi?: IfcApiWithProps, modelID?: number): void {
...
- getTypeNameForElement(ifcApi: IfcAPI, modelID: number, expressID: number): string | null {
+ getTypeNameForElement(ifcApi: IfcApiWithProps, modelID: number, expressID: number): string | null {
...
- getStats(ifcApi: IfcAPI, modelID: number) {
+ getStats(ifcApi: IfcApiWithProps, modelID: number) {Also applies to: 151-156, 157-166, 467-485, 487-507, 509-515, 516-531
🤖 Prompt for AI Agents
In services/selection-index.ts around lines 1 (and also apply to ranges 151-156,
157-166, 467-485, 487-507, 509-515, 516-531), the code uses
ifcApi.properties.getTypeProperties and getPropertySets but the imported IfcAPI
type from "web-ifc" does not declare a .properties namespace, causing TS errors;
create an augmented/widened interface (e.g., extend IfcAPI with a properties
object declaring getTypeProperties and getPropertySets signatures and any used
overloads/return types) and change public method signatures and local variables
that accept IfcAPI to use this augmented type so all calls to .properties.*
type-check; update imports/exports and any function params in the listed line
ranges to use the new widened type and adjust any returned types accordingly.
| const apiToModelIndex = new Map<number, Map<number, InternalIndex>>(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against concurrent builds per (api, model).
Two callers can build the same index simultaneously, doubling work and memory. Add an in-flight registry to dedupe.
Apply this diff:
// Index storage: Map<apiKey, Map<modelID, InternalIndex>>
const apiToModelIndex = new Map<number, Map<number, InternalIndex>>();
+// In-flight build de-dup: Map<apiKey, Map<modelID, Promise<void>>>
+const inFlightBuilds = new Map<number, Map<number, Promise<void>>>();
async build(
- ifcApi: IfcApiWithProps,
+ ifcApi: IfcApiWithProps,
modelID: number,
onProgress?: (percent: number, message?: string) => void,
): Promise<void> {
- const apiKey = getApiId(ifcApi);
+ const apiKey = getApiId(ifcApi);
+ // Dedupe concurrent builds
+ let inflightByModel = inFlightBuilds.get(apiKey);
+ if (!inflightByModel) {
+ inflightByModel = new Map();
+ inFlightBuilds.set(apiKey, inflightByModel);
+ }
+ if (inflightByModel.has(modelID)) return inflightByModel.get(modelID)!;
+ const buildPromise = (async () => {
let modelMap = apiToModelIndex.get(apiKey);
if (!modelMap) {
modelMap = new Map();
apiToModelIndex.set(apiKey, modelMap);
}
if (modelMap.get(modelID)?.ready) return; // Already built
+ // ... remainder of build body ...
+ })();
+ inflightByModel.set(modelID, buildPromise);
+ try {
+ await buildPromise;
+ } finally {
+ inflightByModel.delete(modelID);
+ }
+ return;Also applies to: 157-178, 465-465
1 participant
Summary
Testing
npm test(fails: Missing script: "test")npm run linthttps://chatgpt.com/codex/tasks/task_e_68bfcd38ae2c832083cb631a050cd0b7
Summary by CodeRabbit
New Features
Chores