feat: add select all to property panel #172
Open
Conversation
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds per-property selection to the Model Info UI via a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant MI as ModelInfo / PropertyRow
participant IFC as IFC Context
participant IDX as PropertyIndex (optional)
participant PC as PropertyCache
participant SEL as Selection
User->>MI: Click "select all matching" on a property
MI->>IFC: selectElementsByProperty(selectPath, value)
activate IFC
alt index available
IFC->>IDX: findElementsByProperty(path, value)
IDX-->>IFC: matchedExpressIDs
else no index
IFC->>PC: getBatchProperties(batchOfElementIds)
PC-->>IFC: properties
IFC->>IFC: traverse path & deepEqual -> collect matches
end
IFC->>SEL: selectElements(matchedExpressIDs)
SEL-->>IFC: done
IFC-->>MI: resolved (with matchCount / progress)
deactivate IFC
MI-->>User: Selection updated / progress UI cleared
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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)
89-91: Fix ReactNode concatenation:.join(", ")renders[object Object].
renderPropertyValuemaps to ReactNodes and then string-joins them. That coerces nodes to strings. Render with Fragments and explicit separators instead.- if (Array.isArray(value.values) && value.unit !== undefined) { - // Handles { values: [...], unit: Y } - 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> - </> - ); - } + if (Array.isArray(value.values) && value.unit !== undefined) { + // Handles { values: [...], unit: Y } + return ( + <> + {value.values.map((v: any, i: number) => ( + <React.Fragment key={i}> + {i > 0 && ", "} + {renderPropertyValue(v, keyHint, ifcApi, t)} + </React.Fragment> + ))}{" "} + <span className="text-muted-foreground/80">({value.unit})</span> + </> + ); + }- return value.map((v) => renderPropertyValue(v, keyHint, ifcApi, t)).join(", "); // Render each item + return ( + <> + {value.map((v, i) => ( + <React.Fragment key={i}> + {i > 0 && ", "} + {renderPropertyValue(v, keyHint, ifcApi, t)} + </React.Fragment> + ))} + </> + );Also applies to: 185-186
600-600: GuardgetNaturalIfcClassNamewhenifcTypecan be null on multi-select.Avoids a crash when selected elements have mixed types.
- const naturalIfcInfo = getNaturalIfcClassName(ifcType, lang); + const naturalIfcInfo = ifcType + ? getNaturalIfcClassName(ifcType, lang) + : { name: '', schemaUrl: '' };
🧹 Nitpick comments (9)
public/locales/en/common.json (1)
24-24: Minor grammar fix: add “the”.“Select all elements with same value” → “Select all elements with the same value”.
Apply this diff:
- "selectAllMatching": "Select all elements with same value", + "selectAllMatching": "Select all elements with the same value",context/ifc-context.tsx (2)
139-139: API addition looks good — document expected path roots.Consider documenting expected roots (e.g., ["attributes", ...], ["propertySets", ...]) in the interface JSDoc for consistency across callsites.
1585-1617: Optional fast-path for IFC type equality.If callers pass a path targeting the entity type (e.g., ["attributes","Ifc Class"] or ["ifcType"]), you can bypass property fetch and compare el.type directly for big speedups.
I can draft a guarded fast-path if you share the exact selectPath used in components.
components/model-info.tsx (6)
247-249: Add accessible labels to icon-only “Select all” buttons.Improves screen-reader UX.
- <button onClick={handleSelect} className="opacity-60 hover:opacity-100"> + <button + onClick={handleSelect} + aria-label={t?.('selectAllMatching') ?? 'Select all'} + className="opacity-60 hover:opacity-100" + > <MousePointer2 className="w-3 h-3" /> </button>- <button + <button onClick={(e) => { e.stopPropagation(); selectElementsByProperty(['ifcType'], ifcType); }} + aria-label={t('selectAllMatching')} className="opacity-60 hover:opacity-100" >Also applies to: 631-631
622-638: Avoid nested Tooltips for the IFC Class row.Inner Tooltip inside the outer Tooltip can fight for focus/hover. Consider using a single Tooltip with a richer content area or disable hover events on the inner.
805-807: Localize hardcoded UI strings (“set/sets”, “Model (ID: …)”).Use i18n keys for consistency.
- countUnitSingular="set" - countUnitPlural="sets" + countUnitSingular={t('set')} + countUnitPlural={t('sets')}- countUnitSingular="set" - countUnitPlural="sets" + countUnitSingular={t('set')} + countUnitPlural={t('sets')}- const modelDisplayName = currentModel?.name || `Model (ID: ${modelID})`; + const modelDisplayName = currentModel?.name || t('modelWithId', { id: modelID });Also applies to: 847-849, 591-592
219-220: Clipboard write: add failure handling.Navigator clipboard can be unavailable; ignore or surface a toast on failure.
- const handleCopy = () => { - if (copyValue !== undefined) navigator.clipboard.writeText(copyValue); - }; + const handleCopy = () => { + if (copyValue === undefined) return; + navigator.clipboard.writeText(copyValue).catch(() => {/* no-op */}); + };
40-42: Consider a stable deep-equality helper.
JSON.stringifyis order-sensitive and fails on non-JSON values. Considerlodash.isEqualordequalfor robustness.
244-256: Reduce TooltipProvider duplication.You can wrap the panel once with a single
TooltipProviderinstead of instantiating per-row.Also applies to: 612-612
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
components/model-info.tsx(8 hunks)context/ifc-context.tsx(4 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)
🧰 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:
context/ifc-context.tsxcomponents/model-info.tsx
🔇 Additional comments (5)
public/locales/de/common.json (1)
24-24: LGTM — translation reads natural.No issues spotted for the new key.
public/locales/it/common.json (1)
24-24: LGTM — Italian string is correct and idiomatic.Matches tone of surrounding keys.
public/locales/fr/common.json (1)
24-24: LGTM — French translation is correct.Consistent with existing locale style.
context/ifc-context.tsx (1)
2340-2340: Provider wiring — LGTM.selectElementsByProperty is correctly exposed via context value.
components/model-info.tsx (1)
385-386: Nice: cleanselectPathwiring across sections.Attributes, property sets, and type sets all provide consistent selection paths; matches the new context API.
Also applies to: 747-748, 830-831, 866-867
Comment on lines
+221
to
+225
| const handleSelect = () => { | ||
| if (selectPath) { | ||
| selectElementsByProperty(selectPath, propValue); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize selection payload before calling selectElementsByProperty.
Pass the underlying value(s) for unit-wrapped and {value,type} shapes; skip null/empty.
- const handleSelect = () => {
- if (selectPath) {
- selectElementsByProperty(selectPath, propValue);
- }
- };
+ const handleSelect = () => {
+ if (!selectPath) return;
+ const v = propValue as any;
+ const normalized =
+ v && typeof v === "object"
+ ? Array.isArray(v.values)
+ ? v.values
+ : (v.value ?? v)
+ : v;
+ if (normalized !== undefined && normalized !== null && normalized !== "") {
+ selectElementsByProperty(selectPath, normalized);
+ }
+ };📝 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 handleSelect = () => { | |
| if (selectPath) { | |
| selectElementsByProperty(selectPath, propValue); | |
| } | |
| }; | |
| const handleSelect = () => { | |
| if (!selectPath) return; | |
| const v = propValue as any; | |
| const normalized = | |
| v && typeof v === "object" | |
| ? Array.isArray(v.values) | |
| ? v.values | |
| : (v.value ?? v) | |
| : v; | |
| if (normalized !== undefined && normalized !== null && normalized !== "") { | |
| selectElementsByProperty(selectPath, normalized); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In components/model-info.tsx around lines 221 to 225, the handleSelect callback
passes propValue directly to selectElementsByProperty; normalize the payload
first by unwrapping unit-wrapped values and {value,type} objects to their
underlying primitive(s), supporting both single values and arrays, and filter
out null/undefined/empty-string entries so you never call
selectElementsByProperty with empty or wrapper objects; finally call
selectElementsByProperty(selectPath, normalizedValue) only when normalizedValue
has at least one valid entry.
Comment on lines
+24
to
+26
| function deepEqual(a: any, b: any) { | ||
| return JSON.stringify(a) === JSON.stringify(b); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid JSON.stringify-based deep equality (NaN/-0/cycles pitfalls).
JSON.stringify conflates NaN and null, doesn’t handle -0, and can throw on cycles. Also many IFC values are wrapped as { value, unit }.
Apply this diff to make equality safer and tolerant of common IFC wrappers:
-function deepEqual(a: any, b: any) {
- return JSON.stringify(a) === JSON.stringify(b);
-}
+function deepEqual(a: any, b: any) {
+ // Unwrap simple IFC value wrappers
+ const unwrap = (v: any) =>
+ v && typeof v === "object" && "value" in v ? (v as any).value : v;
+ a = unwrap(a);
+ b = unwrap(b);
+ // Handle NaN and -0 correctly
+ if (typeof a === "number" || typeof b === "number") {
+ if (Number.isNaN(a) && Number.isNaN(b)) return true;
+ return Object.is(a, b);
+ }
+ // Fast path for primitives and reference equality
+ if (a === b) return true;
+ if (
+ a === null ||
+ b === null ||
+ typeof a !== "object" ||
+ typeof b !== "object"
+ ) {
+ return false;
+ }
+ // Best-effort structural compare without bringing a new dep
+ try {
+ return JSON.stringify(a) === JSON.stringify(b);
+ } catch {
+ return false;
+ }
+}📝 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
| function deepEqual(a: any, b: any) { | |
| return JSON.stringify(a) === JSON.stringify(b); | |
| } | |
| function deepEqual(a: any, b: any) { | |
| // Unwrap simple IFC value wrappers | |
| const unwrap = (v: any) => | |
| v && typeof v === "object" && "value" in v ? (v as any).value : v; | |
| a = unwrap(a); | |
| b = unwrap(b); | |
| // Handle NaN and -0 correctly | |
| if (typeof a === "number" || typeof b === "number") { | |
| if (Number.isNaN(a) && Number.isNaN(b)) return true; | |
| return Object.is(a, b); | |
| } | |
| // Fast path for primitives and reference equality | |
| if (a === b) return true; | |
| if ( | |
| a === null || | |
| b === null || | |
| typeof a !== "object" || | |
| typeof b !== "object" | |
| ) { | |
| return false; | |
| } | |
| // Best-effort structural compare without bringing a new dep | |
| try { | |
| return JSON.stringify(a) === JSON.stringify(b); | |
| } catch { | |
| return false; | |
| } | |
| } |
| selectElements: (selection: SelectedElementInfo[]) => void; | ||
| toggleElementSelection: (element: SelectedElementInfo, additive: boolean) => void; | ||
| clearSelection: () => void; | ||
| selectElementsByProperty: (path: string[], value: any) => Promise<void>; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify callsites pass canonical paths/values.
Ensure all calls pass paths rooted at the ParsedElementProperties shape and raw values (not formatted strings with units).
Run:
🏁 Script executed:
#!/bin/bash
# Inspect callsites and passed shapes
rg -n --type=ts --type=tsx -C2 'selectElementsByProperty\\('
rg -n --type=ts --type=tsx -C3 'selectPath\\s*:' | sed -n '1,200p'Length of output: 132
🏁 Script executed:
#!/bin/bash
# Search for callsites of selectElementsByProperty in all .ts and .tsx files
rg -n --glob '*.ts' --glob '*.tsx' -C2 'selectElementsByProperty\(' .
# Search for usages of selectPath property in all .ts and .tsx files
rg -n --glob '*.ts' --glob '*.tsx' -C3 'selectPath\s*:' .Length of output: 706
🏁 Script executed:
#!/bin/bash
# Extract component logic around first callsite
sed -n '180,260p' components/model-info.tsx
# Extract component logic around second callsite
sed -n '600,680p' components/model-info.tsx
# Show context hook destructuring
rg -n "useIfcContext" -C5 components/model-info.tsx
# Show how propValue is defined or updated
rg -n "propValue" -C10 components/model-info.tsx
# Confirm selectPath origin
rg -n "selectPath" -C5 components/model-info.tsxLength of output: 15600
🏁 Script executed:
#!/bin/bash
# Print first 200 lines of context/ifc-context.tsx
sed -n '1,200p' context/ifc-context.tsx
# Find ParsedElementProperties definition
rg -n "ParsedElementProperties" -C5 context/ifc-context.tsx
# Find selectElementsByProperty signature
rg -n "selectElementsByProperty" -C5 context/ifc-context.tsxLength of output: 10945
🏁 Script executed:
#!/bin/bash
# Locate and display the ParsedElementProperties interface definition
rg -n "interface ParsedElementProperties" -C5 .Length of output: 662
Correct selectPath for typeSets usage
- In components/model-info.tsx, update
selectPath={['typeSets', psetName, propName]}
to
selectPath={['propertySets', psetName, propName]}
so it aligns with ParsedElementProperties.propertySets.
🤖 Prompt for AI Agents
In context/ifc-context.tsx around line 139, the review notes that
components/model-info.tsx is passing selectPath=['typeSets', psetName, propName]
but ParsedElementProperties uses propertySets; update usages to
selectPath=['propertySets', psetName, propName] so the selector targets the
correct field. Find all occurrences where 'typeSets' is used for property
selection in that component (or related components) and replace them with
'propertySets', ensuring psetName and propName remain the same and
tests/components referencing the old path are updated accordingly.
Comment on lines
1585
to
1617
| const selectElementsByProperty = useCallback( | ||
| async (path: string[], value: any) => { | ||
| if (!ifcApiInternal) return; | ||
| const matches: SelectedElementInfo[] = []; | ||
| for (const model of loadedModels) { | ||
| if (model.modelID == null) continue; | ||
| const elements = IFCElementExtractor.getAllElements(ifcApiInternal, model.modelID); | ||
| for (const el of elements) { | ||
| try { | ||
| const props = await PropertyCache.getProperties( | ||
| ifcApiInternal, | ||
| model.modelID, | ||
| el.expressID, | ||
| ); | ||
| let current: any = props; | ||
| for (const part of path) { | ||
| if (current == null) break; | ||
| current = current[part as keyof typeof current]; | ||
| } | ||
| if (current !== undefined && deepEqual(current, value)) { | ||
| matches.push({ modelID: model.modelID, expressID: el.expressID }); | ||
| } | ||
| } catch { | ||
| // Ignore property fetch errors | ||
| } | ||
| } | ||
| } | ||
| if (matches.length > 0) { | ||
| selectElements(matches); | ||
| } | ||
| }, | ||
| [ifcApiInternal, loadedModels, selectElements], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make selection more robust and responsive (init properties, normalize values, yield during long scans).
Avoid UI stalls with large models and ensure property helper is initialized; compare normalized values (unwrap {value}).
Apply this diff:
const selectElementsByProperty = useCallback(
async (path: string[], value: any) => {
- if (!ifcApiInternal) return;
+ if (!ifcApiInternal) return;
+ // Ensure properties helper is available
+ if (!ifcApiInternal.properties) {
+ try {
+ ifcApiInternal.properties = new Properties(ifcApiInternal);
+ } catch (e) {
+ console.error("IFCContext: Failed to init properties in selectElementsByProperty", e);
+ return;
+ }
+ }
const matches: SelectedElementInfo[] = [];
+ let processed = 0;
+ // Normalize once for reliable comparisons
+ const unwrap = (v: any) =>
+ v && typeof v === "object" && "value" in v ? (v as any).value : v;
+ const target = unwrap(value);
for (const model of loadedModels) {
if (model.modelID == null) continue;
const elements = IFCElementExtractor.getAllElements(ifcApiInternal, model.modelID);
for (const el of elements) {
try {
const props = await PropertyCache.getProperties(
ifcApiInternal,
model.modelID,
el.expressID,
);
let current: any = props;
for (const part of path) {
if (current == null) break;
current = current[part as keyof typeof current];
}
- if (current !== undefined && deepEqual(current, value)) {
+ if (current !== undefined && deepEqual(unwrap(current), target)) {
matches.push({ modelID: model.modelID, expressID: el.expressID });
}
} catch {
// Ignore property fetch errors
}
+ // Yield periodically to keep UI responsive on large models
+ processed++;
+ if (processed % 250 === 0) {
+ await (async () => new Promise((r) => setTimeout(r, 0)))();
+ }
}
}
if (matches.length > 0) {
selectElements(matches);
}
},
[ifcApiInternal, loadedModels, selectElements],
);📝 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 selectElementsByProperty = useCallback( | |
| async (path: string[], value: any) => { | |
| if (!ifcApiInternal) return; | |
| const matches: SelectedElementInfo[] = []; | |
| for (const model of loadedModels) { | |
| if (model.modelID == null) continue; | |
| const elements = IFCElementExtractor.getAllElements(ifcApiInternal, model.modelID); | |
| for (const el of elements) { | |
| try { | |
| const props = await PropertyCache.getProperties( | |
| ifcApiInternal, | |
| model.modelID, | |
| el.expressID, | |
| ); | |
| let current: any = props; | |
| for (const part of path) { | |
| if (current == null) break; | |
| current = current[part as keyof typeof current]; | |
| } | |
| if (current !== undefined && deepEqual(current, value)) { | |
| matches.push({ modelID: model.modelID, expressID: el.expressID }); | |
| } | |
| } catch { | |
| // Ignore property fetch errors | |
| } | |
| } | |
| } | |
| if (matches.length > 0) { | |
| selectElements(matches); | |
| } | |
| }, | |
| [ifcApiInternal, loadedModels, selectElements], | |
| ); | |
| const selectElementsByProperty = useCallback( | |
| async (path: string[], value: any) => { | |
| if (!ifcApiInternal) return; | |
| // Ensure properties helper is available | |
| if (!ifcApiInternal.properties) { | |
| try { | |
| ifcApiInternal.properties = new Properties(ifcApiInternal); | |
| } catch (e) { | |
| console.error( | |
| "IFCContext: Failed to init properties in selectElementsByProperty", | |
| e, | |
| ); | |
| return; | |
| } | |
| } | |
| const matches: SelectedElementInfo[] = []; | |
| let processed = 0; | |
| // Normalize once for reliable comparisons | |
| const unwrap = (v: any) => | |
| v && typeof v === "object" && "value" in v ? (v as any).value : v; | |
| const target = unwrap(value); | |
| for (const model of loadedModels) { | |
| if (model.modelID == null) continue; | |
| const elements = IFCElementExtractor.getAllElements( | |
| ifcApiInternal, | |
| model.modelID, | |
| ); | |
| for (const el of elements) { | |
| try { | |
| const props = await PropertyCache.getProperties( | |
| ifcApiInternal, | |
| model.modelID, | |
| el.expressID, | |
| ); | |
| let current: any = props; | |
| for (const part of path) { | |
| if (current == null) break; | |
| current = current[part as keyof typeof current]; | |
| } | |
| if ( | |
| current !== undefined && | |
| deepEqual(unwrap(current), target) | |
| ) { | |
| matches.push({ | |
| modelID: model.modelID, | |
| expressID: el.expressID, | |
| }); | |
| } | |
| } catch { | |
| // Ignore property fetch errors | |
| } | |
| // Yield periodically to keep UI responsive on large models | |
| processed++; | |
| if (processed % 250 === 0) { | |
| await (async () => new Promise((r) => setTimeout(r, 0)))(); | |
| } | |
| } | |
| } | |
| if (matches.length > 0) { | |
| selectElements(matches); | |
| } | |
| }, | |
| [ifcApiInternal, loadedModels, selectElements], | |
| ); |
…on progress - Implemented property indexing for faster selections in the IFC model, improving performance during large element searches. - Added selection progress feedback in the viewer to inform users about ongoing operations. - Updated the classification panel to prevent layout shifts and improve user experience with consistent rendering of search results.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
context/ifc-context.tsx (1)
25-27: Replace JSON.stringify equality with value-aware comparison.
JSON.stringify is fragile (NaN/-0/cycles, and IFC {value} wrappers). Use an unwrap + Object.is approach.-function deepEqual(a: any, b: any) { - return JSON.stringify(a) === JSON.stringify(b); -} +function deepEqual(a: any, b: any) { + const unwrap = (v: any) => + v && typeof v === "object" && "value" in v ? (v as any).value : v; + a = unwrap(a); + b = unwrap(b); + if (typeof a === "number" || typeof b === "number") { + if (Number.isNaN(a) && Number.isNaN(b)) return true; + return Object.is(a, b); + } + if (a === b) return true; + if ( + a === null || + b === null || + typeof a !== "object" || + typeof b !== "object" + ) { + return false; + } + try { + return JSON.stringify(a) === JSON.stringify(b); + } catch { + return false; + } +}
🧹 Nitpick comments (9)
components/classification-panel.tsx (4)
149-149: Initialize bsddFeatureSeen from storage to avoid post-mount flickerUse a lazy initializer and a shared key constant; this prevents a re-render flip and centralizes the key.
Apply within this line:
- const [bsddFeatureSeen, setBsddFeatureSeen] = useState(true); // Default to true to prevent initial layout shift + const [bsddFeatureSeen, setBsddFeatureSeen] = useState<boolean>(() => { + try { + return typeof window !== "undefined" && localStorage.getItem(BSDD_SEEN_KEY) === "true"; + } catch { + return true; + } + });Add near other constants (e.g., under SETTINGS_KEY):
const BSDD_SEEN_KEY = "bsddFeatureSeen";
419-427: Use a shared key and tighten logic; skip unnecessary state writesLeverage BSDD_SEEN_KEY and early-return if already seen.
- // Feature highlight: show subtle indicator until user opens it once - try { - const seen = localStorage.getItem("bsddFeatureSeen"); - // Only update state if it's actually false, to prevent unnecessary re-renders - if (seen !== "true") { - setBsddFeatureSeen(false); - } - } catch (e) { + // Feature highlight: show subtle indicator until user opens it once + try { + const seen = localStorage.getItem(BSDD_SEEN_KEY); + if (seen === "true") return; + setBsddFeatureSeen(false); + } catch (e) { // ignore }
1159-1171: Localize the indicator label and respect reduced motionThe “bSDD search” label and “New” badge should be localized; also gate bounce/ping under motion-safe.
- {/* BSDD feature indicator with nice animations */} + {/* BSDD feature indicator with animations (motion-safe) */} {!bsddFeatureSeen && ( <> - {/* Glow ping dot with smooth animation */} - <span className="pointer-events-none absolute -top-1.5 -right-1.5 w-3.5 h-3.5 rounded-full bg-primary/70 animate-ping" /> + {/* Glow ping dot with smooth animation */} + <span className="pointer-events-none absolute -top-1.5 -right-1.5 w-3.5 h-3.5 rounded-full bg-primary/70 motion-safe:animate-ping" /> <span className="pointer-events-none absolute -top-1.5 -right-1.5 w-3.5 h-3.5 rounded-full bg-primary shadow-sm" /> - {/* Floating label with bounce animation - positioned to not affect layout */} - <span className="pointer-events-none select-none absolute right-6 top-full mt-1 bg-primary text-primary-foreground text-[10px] leading-none px-2 py-1 rounded-full shadow-md animate-bounce whitespace-nowrap z-10"> - bSDD search + {/* Floating label with bounce animation - positioned to not affect layout */} + <span className="pointer-events-none select-none absolute right-6 top-full mt-1 bg-primary text-primary-foreground text-[10px] leading-none px-2 py-1 rounded-full shadow-md motion-safe:animate-bounce whitespace-nowrap z-10"> + {t("labels.bsddSearch", "bSDD search")} </span> </> )}Also consider localizing the “New” badge text on Line 1187 similarly.
1733-1755: Localize empty-state strings and add accessibility hintMove the plain English strings into i18n and consider aria-live for the “no results” state.
- <p className="text-xs text-muted-foreground"> - Use the + button above to create your first classification - </p> + <p className="text-xs text-muted-foreground"> + {t("classifications.emptyHint", "Use the + button above to create your first classification")} + </p>components/ifc-model.tsx (2)
919-931: Property index build: add guard, yield, and progress; avoid UI stalls
- Check hasIndex before work (even though buildIndex guards, early-return cuts logs/work).
- Yield once before heavy work to keep the UI responsive.
- // Build property index for fast property-based selections - try { + // Build property index for fast property-based selections + try { + if (PropertyIndex.hasIndex(ifcApi, newIfcModelID)) { + console.log(`IFCModel (${modelData.id}): Property index already present; skipping build.`); + } else { + // Yield to the event loop to keep UI responsive + await new Promise((r) => setTimeout(r, 0)); console.log(`IFCModel (${modelData.id}): Building property index for fast selections...`); - const allElements = IFCElementExtractor.getAllElements(ifcApi, newIfcModelID); - await PropertyIndex.buildIndex(ifcApi, newIfcModelID, allElements, (progress) => { + const allElements = IFCElementExtractor.getAllElements(ifcApi, newIfcModelID); + await PropertyIndex.buildIndex(ifcApi, newIfcModelID, allElements, (progress) => { console.log(`IFCModel (${modelData.id}): Property index building progress: ${progress}%`); - }); - console.log(`IFCModel (${modelData.id}): Property index built successfully`); + }); + console.log(`IFCModel (${modelData.id}): Property index built successfully`); + } } catch (indexError) {
680-711: Clear caches on unmount to avoid memory growthWhen models are closed, clear the element and property indexes for that model.
Add in the cleanup block after CloseModel and before disposing materials:
try { if (ownModelID.current !== null && ifcApi) { PropertyIndex.clearIndex(ownModelID.current, ifcApi); IFCElementExtractor.clearCache(ownModelID.current, ifcApi); } } catch (e) { console.warn(`IFCModel (${modelData.id}): Cache clear warning:`, e); }components/ifc-viewer.tsx (1)
2187-2207: DRY the duplicated progress widgetsRule and selection progress overlays are nearly identical. Extract a small ProgressOverlay component to reduce duplication and keep positioning logic in one place.
context/ifc-context.tsx (2)
263-275: State wiring for selection progress LGTM.
Consider extracting a SelectionProgress type if reused later.
1721-1737: Guard setTimeout cleanup to avoid state updates after unmount.
Minor: track mounted state or clear the timer on unmount to prevent a no-op warning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/classification-panel.tsx(4 hunks)components/ifc-model.tsx(2 hunks)components/ifc-viewer.tsx(4 hunks)context/ifc-context.tsx(7 hunks)services/property-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:
components/ifc-model.tsxcomponents/classification-panel.tsxcomponents/ifc-viewer.tsxservices/property-index.tscontext/ifc-context.tsx
🧬 Code graph analysis (4)
components/ifc-model.tsx (2)
services/ifc-element-extractor.ts (1)
IFCElementExtractor(12-417)services/property-index.ts (1)
PropertyIndex(19-276)
components/ifc-viewer.tsx (1)
components/ui/simple-progress.tsx (1)
SimpleProgress(19-72)
services/property-index.ts (1)
services/property-cache.ts (1)
PropertyCache(7-217)
context/ifc-context.tsx (3)
services/property-index.ts (1)
PropertyIndex(19-276)services/ifc-element-extractor.ts (1)
IFCElementExtractor(12-417)services/property-cache.ts (1)
PropertyCache(7-217)
🔇 Additional comments (11)
components/ifc-model.tsx (2)
11-12: LGTM: imports for element extraction and property indexImports are correct and align with the new indexing flow.
42-45: Unused prop outlineLayeroutlineLayer is accepted but unused in this component. Either wire it to layer assignments or remove to avoid drift.
Would you like me to thread it through to set mesh.layers.enable(outlineLayer) on created meshes?
components/ifc-viewer.tsx (3)
2467-2468: LGTM: i18n usage for upload prompt (saved models path)Text keys look consistent with the rest of the viewer.
2497-2498: LGTM: i18n usage for upload prompt (no saved models path)Consistent localization.
942-942: selectionProgress correctly exported and updated in IFCContextProvider
No changes needed—selectionProgressis defined on the context type, provided byIFCContextProvider, and updated throughoutselectElementsByProperty.context/ifc-context.tsx (5)
18-18: Import looks correct.
PropertyIndex is imported as a named export and used below.
140-140: API surface extension LGTM.
Signature fits usage and returns Promise.
204-209: Progress shape addition LGTM.
Consistent with ruleProgress and exposed later.
2467-2467: Context export LGTM.
Exposing selectElementsByProperty aligns with new UI.
2510-2511: Exposing selectionProgress LGTM.
Matches interface and state.services/property-index.ts (1)
259-275: Stats utility LGTM.
Consistent and useful diagnostics.
Comment on lines
+1605
to
+1744
| const selectElementsByProperty = useCallback( | ||
| async (path: string[], value: any) => { | ||
| if (!ifcApiInternal) return; | ||
|
|
||
| console.log(`[selectElementsByProperty] Searching for ${path.join('.')} = ${value}`); | ||
| const matches: SelectedElementInfo[] = []; | ||
|
|
||
| // Check if this will be a large selection that needs progress indication | ||
| let totalElements = 0; | ||
| for (const model of loadedModels) { | ||
| if (model.modelID == null) continue; | ||
| if (PropertyIndex.hasIndex(ifcApiInternal, model.modelID)) { | ||
| // For indexed models, we can estimate the total quickly | ||
| const allIndexed = PropertyIndex.getAllIndexedExpressIDs(ifcApiInternal, model.modelID); | ||
| totalElements += allIndexed.length; | ||
| } else { | ||
| // For non-indexed models, use element extractor | ||
| const elements = IFCElementExtractor.getAllElements(ifcApiInternal, model.modelID); | ||
| totalElements += elements.length; | ||
| } | ||
| } | ||
|
|
||
| // Show progress for large selections (>1000 elements) | ||
| const showProgress = totalElements > 1000; | ||
| if (showProgress) { | ||
| setSelectionProgress({ | ||
| active: true, | ||
| percent: 0, | ||
| status: `Searching ${totalElements.toLocaleString()} elements for ${path.join('.')} = ${value}`, | ||
| matchCount: 0, | ||
| }); | ||
| } | ||
|
|
||
| for (const model of loadedModels) { | ||
| if (model.modelID == null) continue; | ||
|
|
||
| // Try using the property index first (much faster) | ||
| if (PropertyIndex.hasIndex(ifcApiInternal, model.modelID)) { | ||
| console.log(`[selectElementsByProperty] Using property index for model ${model.modelID}`); | ||
| const indexedExpressIDs = PropertyIndex.findElementsByProperty(ifcApiInternal, model.modelID, path, value); | ||
|
|
||
| for (const expressID of indexedExpressIDs) { | ||
| matches.push({ modelID: model.modelID, expressID }); | ||
| } | ||
|
|
||
| if (showProgress) { | ||
| setSelectionProgress(prev => ({ | ||
| ...prev, | ||
| matchCount: matches.length, | ||
| })); | ||
| } | ||
|
|
||
| console.log(`[selectElementsByProperty] Found ${indexedExpressIDs.length} matches using index`); | ||
| continue; | ||
| } | ||
|
|
||
| // Fallback to the old method if no index exists | ||
| console.log(`[selectElementsByProperty] No index available for model ${model.modelID}, using fallback method`); | ||
| const elements = IFCElementExtractor.getAllElements(ifcApiInternal, model.modelID); | ||
|
|
||
| // For large models, batch process to improve performance | ||
| const batchSize = 500; | ||
| for (let i = 0; i < elements.length; i += batchSize) { | ||
| const batch = elements.slice(i, i + batchSize); | ||
| const expressIDs = batch.map(el => el.expressID); | ||
|
|
||
| try { | ||
| // Use batch property fetching for better performance | ||
| const batchProperties = await PropertyCache.getBatchProperties(ifcApiInternal, model.modelID, expressIDs); | ||
|
|
||
| for (const [expressID, props] of Array.from(batchProperties)) { | ||
| let current: any = props; | ||
| for (const part of path) { | ||
| if (current == null) break; | ||
| current = current[part as keyof typeof current]; | ||
| } | ||
| if (current !== undefined && deepEqual(current, value)) { | ||
| matches.push({ modelID: model.modelID, expressID }); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.warn(`[selectElementsByProperty] Batch processing failed for model ${model.modelID}:`, error); | ||
| // Fall back to individual processing for this batch | ||
| for (const el of batch) { | ||
| try { | ||
| const props = await PropertyCache.getProperties(ifcApiInternal, model.modelID, el.expressID); | ||
| let current: any = props; | ||
| for (const part of path) { | ||
| if (current == null) break; | ||
| current = current[part as keyof typeof current]; | ||
| } | ||
| if (current !== undefined && deepEqual(current, value)) { | ||
| matches.push({ modelID: model.modelID, expressID: el.expressID }); | ||
| } | ||
| } catch { | ||
| // Ignore individual property fetch errors | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Update progress for large selections | ||
| if (showProgress) { | ||
| const percent = Math.round((i + batch.length) / elements.length * 100); | ||
| setSelectionProgress(prev => ({ | ||
| ...prev, | ||
| percent, | ||
| matchCount: matches.length, | ||
| })); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| console.log(`[selectElementsByProperty] Total matches found: ${matches.length}`); | ||
|
|
||
| // Clear progress and select elements | ||
| if (showProgress) { | ||
| setSelectionProgress({ | ||
| active: false, | ||
| percent: 100, | ||
| status: `Found ${matches.length} matching elements`, | ||
| matchCount: matches.length, | ||
| }); | ||
|
|
||
| // Clear progress after a short delay | ||
| setTimeout(() => { | ||
| setSelectionProgress({ | ||
| active: false, | ||
| percent: 0, | ||
| status: "", | ||
| matchCount: 0, | ||
| }); | ||
| }, 2000); | ||
| } | ||
|
|
||
| if (matches.length > 0) { | ||
| selectElements(matches); | ||
| } | ||
| }, | ||
| [ifcApiInternal, loadedModels, selectElements], | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify callsites pass canonical selectPath arrays.
Ensure components use ['attributes','Name'] and ['propertySets', pset, prop] (not 'typeSets'). Run:
🏁 Script executed:
#!/bin/bash
rg -n --type=tsx -C2 'selectPath\s*:' | sed -n '1,200p'Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for call sites of selectElementsByProperty across the codebase
rg -n "selectElementsByProperty\(" -C2Length of output: 686
🏁 Script executed:
#!/bin/bash
# Search for selectPath definitions and usages in the component
rg -n "selectPath" -C3 components/model-info.tsxLength of output: 1648
Use canonical ‘propertySets’ in selectPath
components/model-info.tsx:863 change ['typeSets', psetName, propName] to ['propertySets', psetName, propName]
🤖 Prompt for AI Agents
In context/ifc-context.tsx around lines 1605-1744, the PR note points out a
canonical naming mismatch: update any code that builds property selection paths
to use "propertySets" instead of the incorrect "typeSets"; specifically change
the array ['typeSets', psetName, propName] to ['propertySets', psetName,
propName] in components/model-info.tsx at ~line 863 and ensure any consumers
(selectPath callers, tests, and type definitions) are updated to use the
canonical 'propertySets' key so lookups and selections work consistently.
Comment on lines
+1609
to
+1626
| console.log(`[selectElementsByProperty] Searching for ${path.join('.')} = ${value}`); | ||
| const matches: SelectedElementInfo[] = []; | ||
|
|
||
| // Check if this will be a large selection that needs progress indication | ||
| let totalElements = 0; | ||
| for (const model of loadedModels) { | ||
| if (model.modelID == null) continue; | ||
| if (PropertyIndex.hasIndex(ifcApiInternal, model.modelID)) { | ||
| // For indexed models, we can estimate the total quickly | ||
| const allIndexed = PropertyIndex.getAllIndexedExpressIDs(ifcApiInternal, model.modelID); | ||
| totalElements += allIndexed.length; | ||
| } else { | ||
| // For non-indexed models, use element extractor | ||
| const elements = IFCElementExtractor.getAllElements(ifcApiInternal, model.modelID); | ||
| totalElements += elements.length; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid double-scanning non-indexed models; cache extracted elements during sizing.
You call getAllElements once to count, then again to process. Cache and reuse.
- let totalElements = 0;
+ let totalElements = 0;
+ const nonIndexedElementsByModel = new Map<number, Array<{expressID:number; type:string}>>();
for (const model of loadedModels) {
if (model.modelID == null) continue;
if (PropertyIndex.hasIndex(ifcApiInternal, model.modelID)) {
const allIndexed = PropertyIndex.getAllIndexedExpressIDs(ifcApiInternal, model.modelID);
totalElements += allIndexed.length;
} else {
- const elements = IFCElementExtractor.getAllElements(ifcApiInternal, model.modelID);
+ const elements = IFCElementExtractor.getAllElements(ifcApiInternal, model.modelID);
+ nonIndexedElementsByModel.set(model.modelID, elements);
totalElements += elements.length;
}
}📝 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
| console.log(`[selectElementsByProperty] Searching for ${path.join('.')} = ${value}`); | |
| const matches: SelectedElementInfo[] = []; | |
| // Check if this will be a large selection that needs progress indication | |
| let totalElements = 0; | |
| for (const model of loadedModels) { | |
| if (model.modelID == null) continue; | |
| if (PropertyIndex.hasIndex(ifcApiInternal, model.modelID)) { | |
| // For indexed models, we can estimate the total quickly | |
| const allIndexed = PropertyIndex.getAllIndexedExpressIDs(ifcApiInternal, model.modelID); | |
| totalElements += allIndexed.length; | |
| } else { | |
| // For non-indexed models, use element extractor | |
| const elements = IFCElementExtractor.getAllElements(ifcApiInternal, model.modelID); | |
| totalElements += elements.length; | |
| } | |
| } | |
| console.log(`[selectElementsByProperty] Searching for ${path.join('.')} = ${value}`); | |
| const matches: SelectedElementInfo[] = []; | |
| // Check if this will be a large selection that needs progress indication | |
| let totalElements = 0; | |
| const nonIndexedElementsByModel = new Map<number, Array<{expressID: number; type: string}>>(); | |
| for (const model of loadedModels) { | |
| if (model.modelID == null) continue; | |
| if (PropertyIndex.hasIndex(ifcApiInternal, model.modelID)) { | |
| // For indexed models, we can estimate the total quickly | |
| const allIndexed = PropertyIndex.getAllIndexedExpressIDs(ifcApiInternal, model.modelID); | |
| totalElements += allIndexed.length; | |
| } else { | |
| // For non-indexed models, use element extractor | |
| const elements = IFCElementExtractor.getAllElements(ifcApiInternal, model.modelID); | |
| nonIndexedElementsByModel.set(model.modelID, elements); | |
| totalElements += elements.length; | |
| } | |
| } |
🤖 Prompt for AI Agents
In context/ifc-context.tsx around lines 1609 to 1626, the code calls
IFCElementExtractor.getAllElements for non-indexed models to compute
totalElements and later calls it again when processing those models; avoid
double-scanning by caching the extracted element arrays during this sizing loop.
Create a local Map keyed by model.modelID (or model reference) to store the
elements array when you first call getAllElements, increment totalElements from
the cached array, and later reuse the Map to retrieve the elements for
processing instead of calling getAllElements again; ensure you only call
getAllElements once per non-indexed model and handle null/undefined modelIDs
consistently.
Comment on lines
+1642
to
+1659
| if (PropertyIndex.hasIndex(ifcApiInternal, model.modelID)) { | ||
| console.log(`[selectElementsByProperty] Using property index for model ${model.modelID}`); | ||
| const indexedExpressIDs = PropertyIndex.findElementsByProperty(ifcApiInternal, model.modelID, path, value); | ||
|
|
||
| for (const expressID of indexedExpressIDs) { | ||
| matches.push({ modelID: model.modelID, expressID }); | ||
| } | ||
|
|
||
| if (showProgress) { | ||
| setSelectionProgress(prev => ({ | ||
| ...prev, | ||
| matchCount: matches.length, | ||
| })); | ||
| } | ||
|
|
||
| console.log(`[selectElementsByProperty] Found ${indexedExpressIDs.length} matches using index`); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Indexed path mismatch: normalize paths before querying the index.
PropertyIndex.findElementsByProperty currently only recognizes single-token keys (e.g., "name") and won’t match array paths like ["attributes","Name"]. Normalize before calling, and unwrap rule values.
- if (PropertyIndex.hasIndex(ifcApiInternal, model.modelID)) {
+ if (PropertyIndex.hasIndex(ifcApiInternal, model.modelID)) {
console.log(`[selectElementsByProperty] Using property index for model ${model.modelID}`);
- const indexedExpressIDs = PropertyIndex.findElementsByProperty(ifcApiInternal, model.modelID, path, value);
+ const unwrap = (v: any) =>
+ v && typeof v === "object" && "value" in v ? (v as any).value : v;
+ const canonicalPath =
+ path.length === 2 && path[0] === "attributes"
+ ? `attributes.${path[1]}`
+ : path.length === 1
+ ? path[0]
+ : path.join(".");
+ const indexedExpressIDs = PropertyIndex.findElementsByProperty(
+ ifcApiInternal,
+ model.modelID,
+ // NOTE: requires PropertyIndex to accept string | string[]
+ canonicalPath as any,
+ unwrap(value)
+ );Follow-up: update PropertyIndex.findElementsByProperty to accept string | string[] and flatten internally (see file comments).
📝 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
| if (PropertyIndex.hasIndex(ifcApiInternal, model.modelID)) { | |
| console.log(`[selectElementsByProperty] Using property index for model ${model.modelID}`); | |
| const indexedExpressIDs = PropertyIndex.findElementsByProperty(ifcApiInternal, model.modelID, path, value); | |
| for (const expressID of indexedExpressIDs) { | |
| matches.push({ modelID: model.modelID, expressID }); | |
| } | |
| if (showProgress) { | |
| setSelectionProgress(prev => ({ | |
| ...prev, | |
| matchCount: matches.length, | |
| })); | |
| } | |
| console.log(`[selectElementsByProperty] Found ${indexedExpressIDs.length} matches using index`); | |
| continue; | |
| } | |
| if (PropertyIndex.hasIndex(ifcApiInternal, model.modelID)) { | |
| console.log( | |
| `[selectElementsByProperty] Using property index for model ${model.modelID}` | |
| ); | |
| // Normalize wrapped values | |
| const unwrap = (v: any) => | |
| v && typeof v === "object" && "value" in v ? (v as any).value : v; | |
| // Collapse array paths into a single string | |
| const canonicalPath = | |
| path.length === 2 && path[0] === "attributes" | |
| ? `attributes.${path[1]}` | |
| : path.length === 1 | |
| ? path[0] | |
| : path.join("."); | |
| const indexedExpressIDs = PropertyIndex.findElementsByProperty( | |
| ifcApiInternal, | |
| model.modelID, | |
| // NOTE: requires PropertyIndex to accept string | string[] | |
| canonicalPath as any, | |
| unwrap(value) | |
| ); | |
| for (const expressID of indexedExpressIDs) { | |
| matches.push({ modelID: model.modelID, expressID }); | |
| } | |
| if (showProgress) { | |
| setSelectionProgress((prev) => ({ | |
| ...prev, | |
| matchCount: matches.length, | |
| })); | |
| } | |
| console.log( | |
| `[selectElementsByProperty] Found ${indexedExpressIDs.length} matches using index` | |
| ); | |
| continue; | |
| } |
🤖 Prompt for AI Agents
In context/ifc-context.tsx around lines 1642–1659, the code calls
PropertyIndex.findElementsByProperty with possibly multi-token paths and wrapped
rule values; normalize the path and unwrap values before querying the index:
convert path to a flat string or array (e.g., if path is an array join/flatten
to the index's expected form), and if the rule value is wrapped (e.g., { value:
X } or single-element arrays) extract the raw value(s) to pass to the index;
additionally open a follow-up to update PropertyIndex.findElementsByProperty to
accept string | string[] and internally flatten/normalize incoming paths and
values so callers no longer need to pre-normalize.
Comment on lines
+1661
to
+1714
| // Fallback to the old method if no index exists | ||
| console.log(`[selectElementsByProperty] No index available for model ${model.modelID}, using fallback method`); | ||
| const elements = IFCElementExtractor.getAllElements(ifcApiInternal, model.modelID); | ||
|
|
||
| // For large models, batch process to improve performance | ||
| const batchSize = 500; | ||
| for (let i = 0; i < elements.length; i += batchSize) { | ||
| const batch = elements.slice(i, i + batchSize); | ||
| const expressIDs = batch.map(el => el.expressID); | ||
|
|
||
| try { | ||
| // Use batch property fetching for better performance | ||
| const batchProperties = await PropertyCache.getBatchProperties(ifcApiInternal, model.modelID, expressIDs); | ||
|
|
||
| for (const [expressID, props] of Array.from(batchProperties)) { | ||
| let current: any = props; | ||
| for (const part of path) { | ||
| if (current == null) break; | ||
| current = current[part as keyof typeof current]; | ||
| } | ||
| if (current !== undefined && deepEqual(current, value)) { | ||
| matches.push({ modelID: model.modelID, expressID }); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.warn(`[selectElementsByProperty] Batch processing failed for model ${model.modelID}:`, error); | ||
| // Fall back to individual processing for this batch | ||
| for (const el of batch) { | ||
| try { | ||
| const props = await PropertyCache.getProperties(ifcApiInternal, model.modelID, el.expressID); | ||
| let current: any = props; | ||
| for (const part of path) { | ||
| if (current == null) break; | ||
| current = current[part as keyof typeof current]; | ||
| } | ||
| if (current !== undefined && deepEqual(current, value)) { | ||
| matches.push({ modelID: model.modelID, expressID: el.expressID }); | ||
| } | ||
| } catch { | ||
| // Ignore individual property fetch errors | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Update progress for large selections | ||
| if (showProgress) { | ||
| const percent = Math.round((i + batch.length) / elements.length * 100); | ||
| setSelectionProgress(prev => ({ | ||
| ...prev, | ||
| percent, | ||
| matchCount: matches.length, | ||
| })); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Batch fallback: normalize values and yield during long scans.
- Compare unwrapped values to avoid missing matches.
- Yield after each batch to keep UI responsive.
- const batchSize = 500;
+ const batchSize = 500;
+ const unwrap = (v: any) =>
+ v && typeof v === "object" && "value" in v ? (v as any).value : v;
+ const target = unwrap(value);
for (let i = 0; i < elements.length; i += batchSize) {
const batch = elements.slice(i, i + batchSize);
const expressIDs = batch.map(el => el.expressID);
try {
const batchProperties = await PropertyCache.getBatchProperties(ifcApiInternal, model.modelID, expressIDs);
for (const [expressID, props] of Array.from(batchProperties)) {
let current: any = props;
for (const part of path) {
if (current == null) break;
current = current[part as keyof typeof current];
}
- if (current !== undefined && deepEqual(current, value)) {
+ if (current !== undefined && deepEqual(unwrap(current), target)) {
matches.push({ modelID: model.modelID, expressID });
}
}
} catch (error) {
@@
- if (current !== undefined && deepEqual(current, value)) {
+ if (current !== undefined && deepEqual(unwrap(current), target)) {
matches.push({ modelID: model.modelID, expressID: el.expressID });
}
} catch {
}
}
}
+ // Let the UI breathe on big scans
+ await new Promise((r) => setTimeout(r, 0));
if (showProgress) {
const percent = Math.round((i + batch.length) / elements.length * 100);
setSelectionProgress(prev => ({
...prev,
percent,
matchCount: matches.length,
}));
}
}📝 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
| // Fallback to the old method if no index exists | |
| console.log(`[selectElementsByProperty] No index available for model ${model.modelID}, using fallback method`); | |
| const elements = IFCElementExtractor.getAllElements(ifcApiInternal, model.modelID); | |
| // For large models, batch process to improve performance | |
| const batchSize = 500; | |
| for (let i = 0; i < elements.length; i += batchSize) { | |
| const batch = elements.slice(i, i + batchSize); | |
| const expressIDs = batch.map(el => el.expressID); | |
| try { | |
| // Use batch property fetching for better performance | |
| const batchProperties = await PropertyCache.getBatchProperties(ifcApiInternal, model.modelID, expressIDs); | |
| for (const [expressID, props] of Array.from(batchProperties)) { | |
| let current: any = props; | |
| for (const part of path) { | |
| if (current == null) break; | |
| current = current[part as keyof typeof current]; | |
| } | |
| if (current !== undefined && deepEqual(current, value)) { | |
| matches.push({ modelID: model.modelID, expressID }); | |
| } | |
| } | |
| } catch (error) { | |
| console.warn(`[selectElementsByProperty] Batch processing failed for model ${model.modelID}:`, error); | |
| // Fall back to individual processing for this batch | |
| for (const el of batch) { | |
| try { | |
| const props = await PropertyCache.getProperties(ifcApiInternal, model.modelID, el.expressID); | |
| let current: any = props; | |
| for (const part of path) { | |
| if (current == null) break; | |
| current = current[part as keyof typeof current]; | |
| } | |
| if (current !== undefined && deepEqual(current, value)) { | |
| matches.push({ modelID: model.modelID, expressID: el.expressID }); | |
| } | |
| } catch { | |
| // Ignore individual property fetch errors | |
| } | |
| } | |
| } | |
| // Update progress for large selections | |
| if (showProgress) { | |
| const percent = Math.round((i + batch.length) / elements.length * 100); | |
| setSelectionProgress(prev => ({ | |
| ...prev, | |
| percent, | |
| matchCount: matches.length, | |
| })); | |
| } | |
| } | |
| // Fallback to the old method if no index exists | |
| console.log( | |
| `[selectElementsByProperty] No index available for model ${model.modelID}, using fallback method` | |
| ); | |
| const elements = IFCElementExtractor.getAllElements( | |
| ifcApiInternal, | |
| model.modelID | |
| ); | |
| // For large models, batch process to improve performance | |
| const batchSize = 500; | |
| const unwrap = (v: any) => | |
| v && typeof v === "object" && "value" in v ? (v as any).value : v; | |
| const target = unwrap(value); | |
| for (let i = 0; i < elements.length; i += batchSize) { | |
| const batch = elements.slice(i, i + batchSize); | |
| const expressIDs = batch.map((el) => el.expressID); | |
| try { | |
| // Use batch property fetching for better performance | |
| const batchProperties = await PropertyCache.getBatchProperties( | |
| ifcApiInternal, | |
| model.modelID, | |
| expressIDs | |
| ); | |
| for (const [expressID, props] of Array.from(batchProperties)) { | |
| let current: any = props; | |
| for (const part of path) { | |
| if (current == null) break; | |
| current = current[part as keyof typeof current]; | |
| } | |
| if ( | |
| current !== undefined && | |
| deepEqual(unwrap(current), target) | |
| ) { | |
| matches.push({ modelID: model.modelID, expressID }); | |
| } | |
| } | |
| } catch (error) { | |
| console.warn( | |
| `[selectElementsByProperty] Batch processing failed for model ${model.modelID}:`, | |
| error | |
| ); | |
| // Fall back to individual processing for this batch | |
| for (const el of batch) { | |
| try { | |
| const props = await PropertyCache.getProperties( | |
| ifcApiInternal, | |
| model.modelID, | |
| el.expressID | |
| ); | |
| let current: any = props; | |
| for (const part of path) { | |
| if (current == null) break; | |
| current = current[part as keyof typeof current]; | |
| } | |
| if ( | |
| current !== undefined && | |
| deepEqual(unwrap(current), target) | |
| ) { | |
| matches.push({ | |
| modelID: model.modelID, | |
| expressID: el.expressID, | |
| }); | |
| } | |
| } catch { | |
| // Ignore individual property fetch errors | |
| } | |
| } | |
| } | |
| // Let the UI breathe on big scans | |
| await new Promise((r) => setTimeout(r, 0)); | |
| // Update progress for large selections | |
| if (showProgress) { | |
| const percent = Math.round( | |
| ((i + batch.length) / elements.length) * 100 | |
| ); | |
| setSelectionProgress((prev) => ({ | |
| ...prev, | |
| percent, | |
| matchCount: matches.length, | |
| })); | |
| } | |
| } |
🤖 Prompt for AI Agents
In context/ifc-context.tsx around lines 1661 to 1714, the batch fallback
compares wrapped property values and can miss matches and also blocks the UI
during long scans; update the value comparison to normalize/unwrap both the
fetched property value and the target value before deepEqual (e.g., strip
wrapper objects or arrays to their primitive or canonical form) and after
processing each batch add an await Promise.resolve() (or use setTimeout(...,0)
via a small helper) to yield to the event loop so the UI remains responsive;
ensure the same normalization is applied in both the batchProperties loop and
the individual per-element fallback, and keep the existing progress update logic
after yielding.
Comment on lines
+19
to
+23
| export class PropertyIndex { | ||
| private static index = new Map<string, PropertyIndexEntry[]>(); | ||
| private static apiIds = new WeakMap<IfcAPI, number>(); | ||
| private static nextApiId = 1; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Index design and comment disagree (O(1) claim, O(n) filters).
Current storage is an array per model and uses Array.filter on queries. Either remove the O(1) claim or add value maps to achieve near O(1).
- export class PropertyIndex {
- private static index = new Map<string, PropertyIndexEntry[]>();
+ export class PropertyIndex {
+ // Raw entries (for stats/iteration)
+ private static index = new Map<string, PropertyIndexEntry[]>();
+ // Fast lookup maps per model
+ private static valueMaps = new Map<string, {
+ ifcType?: Map<string, number[]>;
+ name?: Map<string, number[]>;
+ globalId?: Map<string, number[]>;
+ description?: Map<string, number[]>;
+ objectType?: Map<string, number[]>;
+ tag?: Map<string, number[]>;
+ predefinedType?: Map<string, number[]>;
+ }>();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In services/property-index.ts around lines 19 to 23, the class currently stores
PropertyIndexEntry[] per model and the code/comment mismatch: you either must
remove the O(1) claim or make lookups O(1). Replace the array-per-model with a
nested Map structure (e.g., Map<modelKey, Map<propertyName, Map<propertyValue,
Set<PropertyIndexEntry>>>>) or add equivalent value->entry maps alongside the
existing arrays; update the static types (and initialization) accordingly and
change query logic to consult the nested maps for exact-match lookups so
property lookups are near O(1) while keeping fallback scans if needed. Ensure
any insert/delete/update methods maintain all maps consistently.
Comment on lines
+46
to
+62
| static async buildIndex( | ||
| ifcApi: IfcAPI, | ||
| modelID: number, | ||
| elements: Array<{ expressID: number; type: string; typeCode: number }>, | ||
| onProgress?: (progress: number) => void | ||
| ): Promise<void> { | ||
| const cacheKey = this.getCacheKey(modelID, ifcApi); | ||
|
|
||
| // Check if index already exists | ||
| if (this.index.has(cacheKey)) { | ||
| console.log(`[PropertyIndex] Index already exists for model ${modelID}`); | ||
| return; | ||
| } | ||
|
|
||
| console.log(`[PropertyIndex] Building index for ${elements.length} elements in model ${modelID}`); | ||
|
|
||
| const indexEntries: PropertyIndexEntry[] = []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Build per-property maps for fast lookups; normalize stored values.
Populate valueMaps while building the index and normalize (unwrap + strings + case where appropriate).
- const indexEntries: PropertyIndexEntry[] = [];
+ const indexEntries: PropertyIndexEntry[] = [];
+ const maps = {
+ ifcType: new Map<string, number[]>(),
+ name: new Map<string, number[]>(),
+ globalId: new Map<string, number[]>(),
+ description: new Map<string, number[]>(),
+ objectType: new Map<string, number[]>(),
+ tag: new Map<string, number[]>(),
+ predefinedType: new Map<string, number[]>(),
+ };
+ const push = (m: Map<string, number[]>, key: string, id: number) => {
+ const arr = m.get(key) || [];
+ arr.push(id);
+ m.set(key, arr);
+ };
@@
- for (const [expressID, properties] of batchProperties) {
+ const unwrap = (v: any) =>
+ v && typeof v === "object" && "value" in v ? String(v.value) : v == null ? v : String(v);
+ for (const [expressID, properties] of batchProperties) {
const entry: PropertyIndexEntry = { expressID };
-
- // Index the most commonly searched properties
- if (properties.ifcType) entry.ifcType = properties.ifcType;
- if (properties.attributes?.Name?.value) entry.name = String(properties.attributes.Name.value);
- if (properties.attributes?.GlobalId?.value) entry.globalId = String(properties.attributes.GlobalId.value);
- if (properties.attributes?.Description?.value) entry.description = String(properties.attributes.Description.value);
- if (properties.attributes?.ObjectType?.value) entry.objectType = String(properties.attributes.ObjectType.value);
- if (properties.attributes?.Tag?.value) entry.tag = String(properties.attributes.Tag.value);
- if (properties.attributes?.PredefinedType?.value) entry.predefinedType = String(properties.attributes.PredefinedType.value);
+ if (properties.ifcType) {
+ entry.ifcType = String(properties.ifcType).toUpperCase();
+ push(maps.ifcType, entry.ifcType, expressID);
+ }
+ const name = unwrap(properties.attributes?.Name);
+ if (name) {
+ entry.name = String(name);
+ push(maps.name, entry.name, expressID);
+ }
+ const gid = unwrap(properties.attributes?.GlobalId);
+ if (gid) {
+ entry.globalId = String(gid);
+ push(maps.globalId, entry.globalId, expressID);
+ }
+ const desc = unwrap(properties.attributes?.Description);
+ if (desc) {
+ entry.description = String(desc);
+ push(maps.description, entry.description, expressID);
+ }
+ const objType = unwrap(properties.attributes?.ObjectType);
+ if (objType) {
+ entry.objectType = String(objType);
+ push(maps.objectType, entry.objectType, expressID);
+ }
+ const tag = unwrap(properties.attributes?.Tag);
+ if (tag) {
+ entry.tag = String(tag);
+ push(maps.tag, entry.tag, expressID);
+ }
+ const predef = unwrap(properties.attributes?.PredefinedType);
+ if (predef) {
+ entry.predefinedType = String(predef);
+ push(maps.predefinedType, entry.predefinedType, expressID);
+ }
indexEntries.push(entry);
}
@@
- this.index.set(cacheKey, indexEntries);
+ this.index.set(cacheKey, indexEntries);
+ this.valueMaps.set(cacheKey, maps);Also applies to: 65-97
🤖 Prompt for AI Agents
In services/property-index.ts around lines 46-62 and continuing through 65-97,
the index builder currently does not populate per-property valueMaps nor
normalize stored property values for fast lookups; update the loop that creates
PropertyIndexEntry objects to build a valueMap for each property: for each
property value, unwrap arrays/optional wrappers, convert to a canonical string
(trim, toLowerCase for case-insensitive fields, or preserve numeric types when
appropriate), deduplicate via a Set, and store the final normalized values array
on the entry (and maintain a map of normalized value -> original raw values if
needed). Ensure keys in the map use a stable identifier (property name +
type/code), populate the entry.valueMaps and any global lookup maps before
pushing indexEntries, and then persist the completed index into this.index so
subsequent lookups use the normalized, deduplicated value lists.
Comment on lines
+107
to
+180
| static findElementsByProperty( | ||
| ifcApi: IfcAPI, | ||
| modelID: number, | ||
| propertyPath: string[], | ||
| value: any | ||
| ): number[] { | ||
| const cacheKey = this.getCacheKey(modelID, ifcApi); | ||
| const indexEntries = this.index.get(cacheKey); | ||
|
|
||
| if (!indexEntries) { | ||
| console.warn(`[PropertyIndex] No index found for model ${modelID}, falling back to cache`); | ||
| return []; | ||
| } | ||
|
|
||
| // Handle common property paths efficiently | ||
| if (propertyPath.length === 1) { | ||
| const propertyName = propertyPath[0]; | ||
|
|
||
| switch (propertyName) { | ||
| case 'ifcType': | ||
| return indexEntries | ||
| .filter(entry => entry.ifcType === value) | ||
| .map(entry => entry.expressID); | ||
|
|
||
| case 'attributes.Name': | ||
| case 'name': | ||
| return indexEntries | ||
| .filter(entry => entry.name === value) | ||
| .map(entry => entry.expressID); | ||
|
|
||
| case 'attributes.GlobalId': | ||
| case 'globalId': | ||
| return indexEntries | ||
| .filter(entry => entry.globalId === value) | ||
| .map(entry => entry.expressID); | ||
|
|
||
| case 'attributes.Description': | ||
| case 'description': | ||
| return indexEntries | ||
| .filter(entry => entry.description === value) | ||
| .map(entry => entry.expressID); | ||
|
|
||
| case 'attributes.ObjectType': | ||
| case 'objectType': | ||
| return indexEntries | ||
| .filter(entry => entry.objectType === value) | ||
| .map(entry => entry.expressID); | ||
|
|
||
| case 'attributes.Tag': | ||
| case 'tag': | ||
| return indexEntries | ||
| .filter(entry => entry.tag === value) | ||
| .map(entry => entry.expressID); | ||
|
|
||
| case 'attributes.PredefinedType': | ||
| case 'predefinedType': | ||
| return indexEntries | ||
| .filter(entry => entry.predefinedType === value) | ||
| .map(entry => entry.expressID); | ||
| } | ||
| } | ||
|
|
||
| // For IFC class searches, we can do early filtering by type | ||
| if (propertyPath.length === 1 && propertyPath[0] === 'ifcType') { | ||
| console.log(`[PropertyIndex] Using optimized IFC type filtering for ${value}`); | ||
| return indexEntries | ||
| .filter(entry => entry.ifcType === value) | ||
| .map(entry => entry.expressID); | ||
| } | ||
|
|
||
| // For other property paths, we need to fall back to cache | ||
| console.log(`[PropertyIndex] Property path ${propertyPath.join('.')} not indexed, using cache fallback`); | ||
| return []; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bug: findElementsByProperty assumes single-token path. Accept string | string[] and flatten; unwrap and normalize value.
Without this, calls like ["attributes","Name"] won’t hit the index.
- static findElementsByProperty(
- ifcApi: IfcAPI,
- modelID: number,
- propertyPath: string[],
- value: any
- ): number[] {
+ static findElementsByProperty(
+ ifcApi: IfcAPI,
+ modelID: number,
+ propertyPath: string[] | string,
+ value: any
+ ): number[] {
const cacheKey = this.getCacheKey(modelID, ifcApi);
- const indexEntries = this.index.get(cacheKey);
+ const indexEntries = this.index.get(cacheKey);
+ const maps = this.valueMaps.get(cacheKey);
@@
- if (!indexEntries) {
+ if (!indexEntries || !maps) {
console.warn(`[PropertyIndex] No index found for model ${modelID}, falling back to cache`);
return [];
}
- // Handle common property paths efficiently
- if (propertyPath.length === 1) {
- const propertyName = propertyPath[0];
+ const flatten = (p: string[] | string) =>
+ Array.isArray(p)
+ ? (p.length === 2 && p[0] === 'attributes' ? `attributes.${p[1]}` : p.join('.'))
+ : p;
+ const propertyName = flatten(propertyPath);
+ const unwrap = (v: any) =>
+ v && typeof v === "object" && "value" in v ? (v as any).value : v;
+ const raw = unwrap(value);
+ const str = raw == null ? '' : String(raw);
+ const upper = String(raw ?? '').toUpperCase();
- switch (propertyName) {
+ switch (propertyName) {
case 'ifcType':
- return indexEntries
- .filter(entry => entry.ifcType === value)
- .map(entry => entry.expressID);
+ return (maps.ifcType?.get(upper) ?? []);
- case 'attributes.Name':
- case 'name':
- return indexEntries
- .filter(entry => entry.name === value)
- .map(entry => entry.expressID);
+ case 'attributes.Name':
+ case 'Name':
+ case 'name':
+ return (maps.name?.get(str) ?? []);
- case 'attributes.GlobalId':
- case 'globalId':
- return indexEntries
- .filter(entry => entry.globalId === value)
- .map(entry => entry.expressID);
+ case 'attributes.GlobalId':
+ case 'GlobalId':
+ case 'globalId':
+ return (maps.globalId?.get(str) ?? []);
- case 'attributes.Description':
- case 'description':
- return indexEntries
- .filter(entry => entry.description === value)
- .map(entry => entry.expressID);
+ case 'attributes.Description':
+ case 'Description':
+ case 'description':
+ return (maps.description?.get(str) ?? []);
- case 'attributes.ObjectType':
- case 'objectType':
- return indexEntries
- .filter(entry => entry.objectType === value)
- .map(entry => entry.expressID);
+ case 'attributes.ObjectType':
+ case 'ObjectType':
+ case 'objectType':
+ return (maps.objectType?.get(str) ?? []);
- case 'attributes.Tag':
- case 'tag':
- return indexEntries
- .filter(entry => entry.tag === value)
- .map(entry => entry.expressID);
+ case 'attributes.Tag':
+ case 'Tag':
+ case 'tag':
+ return (maps.tag?.get(str) ?? []);
- case 'attributes.PredefinedType':
- case 'predefinedType':
- return indexEntries
- .filter(entry => entry.predefinedType === value)
- .map(entry => entry.expressID);
+ case 'attributes.PredefinedType':
+ case 'PredefinedType':
+ case 'predefinedType':
+ return (maps.predefinedType?.get(str) ?? []);
- }
- }
+ }
- // For IFC class searches, we can do early filtering by type
- if (propertyPath.length === 1 && propertyPath[0] === 'ifcType') {
- console.log(`[PropertyIndex] Using optimized IFC type filtering for ${value}`);
- return indexEntries
- .filter(entry => entry.ifcType === value)
- .map(entry => entry.expressID);
- }
+ // Not indexed (e.g., propertySets.*). Use cache fallback in caller.
+ return [];Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In services/property-index.ts around lines 107 to 180, findElementsByProperty
currently only handles single-token propertyPath and misses calls like
["attributes","Name"]; change the signature to accept propertyPath: string |
string[], normalize it to a flattened string[] (split dot-strings and collapse
arrays), then create a normalizedPath string by joining with '.' for index
lookups; also normalize the search value (unwrap primitive from wrapper objects,
coerce case for name/globalId comparisons if index uses normalized casing) and
compare against the indexed fields using both the single-token variants and the
joined dotted-key (e.g., "attributes.Name" and "attributes.Name" normalized) so
that paths provided as ["attributes","Name"] or "attributes.Name" match existing
index entries; keep existing optimized ifcType branch but base it on the
normalizedPath.
Comment on lines
+224
to
+254
| /** | ||
| * Clear index for specific model or all models | ||
| */ | ||
| static clearIndex(modelID?: number, ifcApi?: IfcAPI): void { | ||
| if (modelID !== undefined || ifcApi !== undefined) { | ||
| const keysToDelete: string[] = []; | ||
| const apiId = ifcApi ? this.getApiId(ifcApi) : null; | ||
|
|
||
| for (const key of Array.from(this.index.keys())) { | ||
| let shouldDelete = false; | ||
|
|
||
| if (ifcApi !== undefined && modelID !== undefined) { | ||
| shouldDelete = key === `${apiId}-${modelID}`; | ||
| } else if (modelID !== undefined) { | ||
| shouldDelete = key.includes(`-${modelID}`) || key === modelID.toString(); | ||
| } else if (ifcApi !== undefined) { | ||
| shouldDelete = key.startsWith(`${apiId}-`); | ||
| } | ||
|
|
||
| if (shouldDelete) { | ||
| keysToDelete.push(key); | ||
| } | ||
| } | ||
|
|
||
| for (const key of keysToDelete) { | ||
| this.index.delete(key); | ||
| } | ||
| } else { | ||
| this.index.clear(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clear both raw and value maps when clearing.
Otherwise stale value maps linger.
- for (const key of keysToDelete) {
- this.index.delete(key);
- }
+ for (const key of keysToDelete) {
+ this.index.delete(key);
+ this.valueMaps.delete(key);
+ }
} else {
this.index.clear();
+ this.valueMaps.clear();
}📝 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
| /** | |
| * Clear index for specific model or all models | |
| */ | |
| static clearIndex(modelID?: number, ifcApi?: IfcAPI): void { | |
| if (modelID !== undefined || ifcApi !== undefined) { | |
| const keysToDelete: string[] = []; | |
| const apiId = ifcApi ? this.getApiId(ifcApi) : null; | |
| for (const key of Array.from(this.index.keys())) { | |
| let shouldDelete = false; | |
| if (ifcApi !== undefined && modelID !== undefined) { | |
| shouldDelete = key === `${apiId}-${modelID}`; | |
| } else if (modelID !== undefined) { | |
| shouldDelete = key.includes(`-${modelID}`) || key === modelID.toString(); | |
| } else if (ifcApi !== undefined) { | |
| shouldDelete = key.startsWith(`${apiId}-`); | |
| } | |
| if (shouldDelete) { | |
| keysToDelete.push(key); | |
| } | |
| } | |
| for (const key of keysToDelete) { | |
| this.index.delete(key); | |
| } | |
| } else { | |
| this.index.clear(); | |
| } | |
| } | |
| /** | |
| * Clear index for specific model or all models | |
| */ | |
| static clearIndex(modelID?: number, ifcApi?: IfcAPI): void { | |
| if (modelID !== undefined || ifcApi !== undefined) { | |
| const keysToDelete: string[] = []; | |
| const apiId = ifcApi ? this.getApiId(ifcApi) : null; | |
| for (const key of Array.from(this.index.keys())) { | |
| let shouldDelete = false; | |
| if (ifcApi !== undefined && modelID !== undefined) { | |
| shouldDelete = key === `${apiId}-${modelID}`; | |
| } else if (modelID !== undefined) { | |
| shouldDelete = key.includes(`-${modelID}`) || key === modelID.toString(); | |
| } else if (ifcApi !== undefined) { | |
| shouldDelete = key.startsWith(`${apiId}-`); | |
| } | |
| if (shouldDelete) { | |
| keysToDelete.push(key); | |
| } | |
| } | |
| for (const key of keysToDelete) { | |
| this.index.delete(key); | |
| this.valueMaps.delete(key); | |
| } | |
| } else { | |
| this.index.clear(); | |
| this.valueMaps.clear(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In services/property-index.ts around lines 224 to 254, the clearIndex
implementation only removes entries from one internal map leaving the
corresponding "value" map stale; modify the logic to delete the same keys from
both underlying maps when removing specific entries (e.g., delete from
this.rawIndex and this.valueIndex or whichever two maps hold raw and
derived/value data), and in the else branch clear both maps instead of only one
so both raw and value maps are fully cleared.
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
Testing
npm test(fails: Missing script)npm run linthttps://chatgpt.com/codex/tasks/task_e_68bfcd38ae2c832083cb631a050cd0b7
Summary by CodeRabbit
New Features
Improvements
Chores