feat: TF-IDF entry predicate scoring, avg height metric, pino logger, and triple import fixes - #31
Conversation
… all vite logger imports
Tooltip now shows abstract formula first, then each term's weight × value = contribution on separate lines. Uses monospace HTML formatting for alignment. Last commit changed coefficients (B=2, D=0, F=1.2) but skipped tooltip update; this brings them in sync.
…lean up dead code
MultiSelectInput.svelte — deduplicate options prop
Add `deduplicatedOptions = $derived([...new Set(options)])` before filtering
to prevent Svelte `each_key_duplicate` error when the same IRI appears
multiple times in the options array (e.g., rdfs:seeAlso at indexes 0 and 7).
Triple.ts — rewrite upsertMany for batch insert performance
Replace per-triple `exists()` checks (one DB query per triple) with batch
`insertMany({ ordered: false })` on named and literal discriminators.
Use `Promise.allSettled` and ignore duplicate-key errors (code 11000) to
handle idempotent inserts efficiently. This eliminates O(n) round-trips
for large triple imports.
update-crawling-results/+server.ts — fix N3 async parser + batch NTriples
The N3 Parser operates asynchronously when a callback is supplied;
`_lexer.tokenize(input, processNextToken)` schedules callbacks via
microtasks. The previous refactored code checked `batch.length` immediately
after `new Parser().parse(chunk, callback)` returned — before any callbacks
fired — causing every batch to be empty and 0 triples to be imported.
Wrap each parse call in a Promise that resolves on the `null` quad
completion signal, matching the original code's pattern.
Additionally, process N-Triples in chunks of 5000 lines to reduce peak
memory usage during decompression + parsing of large gzipped payloads.
+page.server.ts — remove duplicate updateCrawlingResults form action
This form action duplicated the logic in the standalone API endpoint at
`/api/subgraphs/[sgid]/update-crawling-results`. The UI button in
`+page.svelte` already calls the API endpoint directly via fetch, so the
form action was dead code. Remove unused imports (Triple, TripleType,
getDerzisTriples, gunzipSync, Parser, Actions type).
package.json — increase Node memory limit for dev server
Set `NODE_OPTIONS='--max-old-space-size=8192'` in the dev script to
prevent OOM crashes when the Vite dev server processes large graph data.
…i recompute The subgraph page server was recomputing TF-IDF scores for all entry predicates on every page visit via three full-table-scan SPARQL queries (getPredicateCounts, getPredicateDistinctResources, getSeedPredicateCoverage). TF-IDF is already computed during the scoring pipeline in computeOnce() and used in the pair score formula, but was never stored in the database. Fix: - Add entryPredTfIdf field to PredicatePairClass model (SubGraph.ts) - Add entryPredTfIdf to the local PredicatePair interface and store the value when creating pairs in computeOnce() (metrics.ts) - Remove computePredicateTfIdf call and its SPARQL queries from the page load function (page.server.ts). The TF-IDF value is now read directly from the stored pair document alongside all other pair data.
The SCORE_A..SCORE_F constants were defined in two independent locations (metrics.ts and SubGraphStepDetails.svelte), creating a risk of divergence. Move them to a new $lib/predicates/constants.ts that has no server-only imports (no Mongoose, no Fuseki), making it safely importable from both the server-side metrics module and the client-side Svelte component.
The BFS in longestPathFromRoots never tracked which nodes had been dequeued, so in cyclic subgraphs (single-SCC branch of computeComponentHeight) the same node could be pushed to the queue repeatedly without bound, causing an infinite loop. Add a visited set — skip nodes that have already been processed.
The previous code passed all unique triples to insertMany in a single call. For large inserts (e.g. 1.6M triples during crawling updates), this could exhaust MongoDB memory or exceed the 16MB document limit. Split each type (named/literal) into batches of 1000, matching the existing pattern in the import API endpoint (api/import/+server.ts).
computePredicateTfIdf was a SPARQL-fetching wrapper used by +page.server.ts before Fix 1 (DB-persisted TF-IDF). It has no production caller — computeOnce queries Fuseki directly and calls computePredicateTfIdfScores. computeLCR was a standalone Union-Find LCR computation with tests but no production caller. computeLCRAndAvgHeight is the function used in the scoring pipeline at line 204.
andrefs
left a comment
andrefs
left a comment
There was a problem hiding this comment.
Comprehensive Code Review — PR #31 (score-params)
Bugs
1. longestPathFromRoots: visited-set prevents propagation of longer paths
File: src/lib/predicates/metrics.ts:579-614
When a node is discovered via a shorter path first, processed, and then a longer path to the same node is found, the visited set causes the longer depth to be silently dropped — descendants of that node never see the longer distance. The depth is updated (depth.set(next, nextDepth)) but the node is not re-visited, so the longer path never propagates to children.
Example graph: A → X and B → C → X. If X is popped first (depth=1 from A), then when C is processed and finds X with depth=2, visited.has(X) is true and X's outgoing edges are never re-traversed. Any descendants of X receive depth 2 instead of 3.
Since this function is only called when sccCount === 1 (unusual for a real DAG), this may not trigger often, but it is a correctness bug waiting to surface. Recommend rewriting this as a proper topological-sort DP or using Kahn's algorithm without the visited early-exit.
2. BulkWriteError code 11000 check is insufficient
File: src/lib/models/Triple.ts:94-97
const err = result.reason as Error & { code?: number };
if (err.code !== 11000) throw err;With ordered: false, Mongoose may throw a BulkWriteError containing multiple error types. The err.code property reflects only the first error, not all of them. If the batch contains a mix of duplicate-key errors (11000) and validation errors (121), this check could either:
- Silently swallow validation failures, or
- Incorrectly throw when only some operations were 11000 but
err.codehappens to be from a non-11000 operation.
Fix: Check err.writeErrors (array of individual write errors) and only swallow the error if every write error has code: 11000.
3. computeLCRAndAvgHeight union-find may compute wrong LCR for duplicate edges
File: src/lib/predicates/metrics.ts:410-437
The union-find correctly ignores duplicate edges from DISTINCT queries, but the adjacency list doesn't deduplicate: if the SPARQL DISTINCT ever returns the same (s, o) pair, it's added to adjacency twice, inflating both indegree and traversal counts. The computeComponentHeight SCC algorithm could produce incorrect results for duplicate edges. If DISTINCT in the SPARQL guarantees uniqueness this is a non-issue, but it's brittle.
Performance
4. Duplicate Fuseki queries: computeOnce vs page load
Files: src/lib/predicates/metrics.ts:105-106 and src/routes/subgraphs/[sgid]/+page.server.ts:55
computeOnce fetches getPredicateDistinctResources and getSeedPredicateCoverage, passes them through computePredicateTfIdfScores, and stores results in MongoDB. Then +page.server.ts calls computePredicateTfIdf() which independently fetches the same two SPARQL queries again. For large datasets, this doubles Fuseki load every time the page is viewed after metrics are computed.
These results are already stored in the PredicatePair documents (via entryPredTfIdf). The page load should read from MongoDB instead of re-querying Fuseki.
5. insertMany batches launched in parallel
File: src/lib/models/Triple.ts:77-90
Promise.allSettled fires all insertMany batches simultaneously. For 10K triples with BATCH_SIZE=1000, this means 10 concurrent insertMany calls hitting MongoDB in parallel. This can overwhelm the connection pool and cause MongooseError: Operation timed out. Consider processing batches sequentially (e.g., with a for loop) or limiting concurrency.
6. SPARQL UNION in getSeedPredicateCoverage may be expensive
File: src/lib/fuseki/fuseki-client.ts:188-200
The UNION of two VALUES patterns forces Fuseki to scan the entire dataset twice. For datasets with millions of triples, this could be very slow. Consider using FILTER-based alternatives or Fuseki-specific optimizations (query hints, sub-select).
7. Logger formatArgs inspects every call site, even when level is suppressed
File: src/lib/logger.ts:73-76
const formatArgs = (msg: string, ...args: unknown[]) => {
if (args.length === 0) return msg;
return msg + ' ' + args.map((a) => util.inspect(a, { colors: true })).join(' ');
};util.inspect is called eagerly for every argument on every log call, even if the log level would suppress the output. For frequent debug/silly calls in hot paths, this is wasted CPU. Pino supports a lazy msg function pattern — consider using pino's native structured logging instead of string formatting.
Type Safety
8. entryPredTfIdf: required in TypeScript interface, optional in Mongoose schema
PredicatePairinterface (metrics.ts:382):entryPredTfIdf: number(required)PredicatePairClass(SubGraph.ts:242-243):@prop({ required: false, type: Number })(optional)PairLeanin+page.server.ts:entryPredTfIdf?: number(optional)
This creates a hole: anyone reading a PredicatePair document that predates this PR will find entryPredTfIdf is undefined, but TypeScript thinks it's always number. Use number | undefined in the interface, or add a migration to backfill the field.
9. customTransport interface mismatch
File: src/lib/logger.ts:36-37
The type annotation says write(chunk: string | Buffer): boolean but the actual method signature is write(chunk: unknown): boolean. This widens the parameter type — TypeScript will accept this because of method signature compatibility rules, but it's technically incorrect and can hide bugs if pino ever passes non-string/Buffer chunks.
Dead Code
10. Unused computePredicateTfIdf exported function not used in computeOnce
computePredicateTfIdf is a convenience wrapper that fetches from Fuseki and calls computePredicateTfIdfScores. The computeOnce function calls computePredicateTfIdfScores directly with manually fetched data. The wrapper is only used by +page.server.ts. This is not dead per se, but it duplicates the Fuseki-fetching logic, making the codebase harder to maintain. Consider having computeOnce call computePredicateTfIdf and passing the result through.
Style & Conventions
11. Score constants live in a separate file — good
Moving SCORE_A–SCORE_F to constants.ts is a clean separation. However, the import path is $lib/predicates/constants while the file is src/lib/predicates/constants.ts — please confirm $lib is configured to resolve this correctly.
12. Logger child naming convention is inconsistent
Most modules use kebab-case (predicates-metrics, derzis-api), but some use colons (webhook:global-metrics, webhook:labels-fetched, api:webhook). Pick one convention and stick to it. The colon-delimited format reads better in logs for hierarchical namespacing.
13. TypeScript return type annotations
The createLogger return type (MonkeyPatchedLogger) is explicit — good. But computePredicateTfIdfScores, computeLCRAndAvgHeight, and computeComponentHeight lack explicit return types where inference is opaque. Add them.
Hardcoded Values
14. BATCH_LINES = 5000 in update-crawling-results/+server.ts
File: src/routes/api/subgraphs/[sgid]/update-crawling-results/+server.ts:9
Hardcoded. For very large N-Triples dumps, 5000 lines may be too small (many small batches, each creating a new Parser). For small dumps, it adds overhead. Consider making this configurable via environment variable with a sensible default.
15. HIER_PRED_MIN_COUNT = 10 in metrics.ts
File: src/lib/predicates/metrics.ts:10
Hardcoded. Consider making this configurable or at least documented.
16. BATCH_SIZE = 1000 in Triple.ts
File: src/lib/models/Triple.ts:76
Also hardcoded. Same concern.
Test Coverage Gaps
17. longestPathFromRoots is completely untested
No direct test for this function. Only tested transitively through computeLCRAndAvgHeight, and only when sccCount > 1 (i.e., DAG cases). The sccCount === 1 branch (where longestPathFromRoots is called) has no test coverage.
18. computePredicateTfIdfScores edge cases not tested
effectiveNumSeeds = 0(what happens when seed coverage is empty?)totalTriples = 0(division by infinity guard exists but untested)- Predicate present in
seedCoveragesbut not inpredicateCounts
19. Logger tests only check "doesn't throw"
File: src/lib/logger.spec.ts
Does not verify:
- Actual log output format
- Level filtering (e.g.,
debugmessages not emitted whenLOG_LEVEL=info) - Colorette color codes in the output
formatArgsbehavior with splat arguments
20. Triple.upsertMany regression test would be valuable
The rewrite from per-triple exists() checks to batch insertMany({ ordered: false }) changes behavior on duplicate keys. No test verifies:
- Deduplication within the input array works
- Existing triples are silently skipped (not duplicated)
- Mixed batches (namedNode + literal) are handled atomically
Summary
This PR makes significant improvements: the N3 parser async fix is critical, the winston→pino refactor is well-executed, and the new scoring metrics add real value. However, I recommend addressing the visited-set bug in longestPathFromRoots and the BulkWriteError code check before merging. The duplicate Fuseki queries and parallel MongoDB batches are important performance concerns for production-scale datasets.
Severity breakdown:
…path The BFS with visited set could produce incorrect heights in multi-path DAGs: if a node was reached via a short path first and marked visited, a longer path discovered later was stored in depth but never propagated to descendants. Replace with Kahn's algorithm — nodes are enqueued only when their in-degree reaches zero, guaranteeing all predecessors have been processed and the maximum depth to each node is final before its children are visited. Cycles are naturally excluded (nodes never reach in-degree zero).
…kWriteError With ordered: false, MongoDB's BulkWriteError can contain a mix of error types. The top-level err.code reflects only the first operation error. If a batch had duplicates (11000) alongside validation errors (121), the old check could either silently swallow real failures or incorrectly re-throw valid duplicate-key errors. Check err.writeErrors — only swallow the entire error when every individual write error has code 11000.
…g Mongo Promise.allSettled over all 1000-item batches launched all insertMany calls concurrently. For 10K triples this meant 10 parallel operations hitting MongoDB, risking connection pool exhaustion. Replace with sequential for loops — named batches first, then literal batches. Each batch awaits completion before the next begins.
…lti-path DAG height - New constants.test.ts verifies all SCORE_A..SCORE_F coefficients - TF-IDF edge cases: totalTriples=0, effectiveNumSeeds=0, predicate in seedCoverages but not in counts, unknown numSeeds with empty coverage - computeLCRAndAvgHeight: diamond/multi-path DAG patterns (the case the Kahn's DP replacement was designed for), multi-component with different heights, detached cycles
…ndAvgHeight
Duplicate (s,o) pairs would inflate indegree counts and create redundant
entries in the adjacency map, affecting SCC decomposition and longest-path
computation.
Guard the edge-processing loop with a seenEdges Set keyed on `${s}|${o}`.
formatArgs called util.inspect on every argument eagerly, even when the log level suppressed the output. Wrap in a lambda so pino only evaluates it when the level is active.
The type annotation claimed write accepted string | Buffer but the implementation used unknown. Changed to match runtime.
8 modules used kebab-case names (derzis-api, predicates-metrics) while webhook and route modules used colon-delimited names (webhook:labels-fetched, api:subgraph:prefixes). Align all to colons for consistent hierarchical namespacing in log output.
…add output format tests Pino v8 does not support function-based messages — passing () => string as msg silently drops the message with no output. Revert to eager util.inspect evaluation. Add 8 tests verifying output format: module name, log level label, ISO timestamp, splat arg inspection, object formatting, and no-arg handling.
andrefs
left a comment
andrefs
left a comment
There was a problem hiding this comment.
Review of PR #31 — TF-IDF scoring, avg height, pino logger, triple import fixes
I reviewed all 33 files changed across 18 commits. Overall this is a well-structured PR with thorough testing and thoughtful commit history. Below are categorized findings.
Critical
C1: Recursion in Tarjan's SCC algorithm may cause stack overflow on large components
src/lib/predicates/metrics.ts:489–516
The strongConnect inner function implements Tarjan's SCC algorithm recursively. SPARQL getPredicateEdges can return unbounded results — for example, an rdfs:subClassOf chain with 50K+ nodes would recurse 50K+ levels deep. The Node.js default call stack limit is ~10K frames on most platforms.
Recommendation: Replace with an iterative implementation of Tarjan's SCC using an explicit stack, or add a recursion-depth guard that falls back to computeComponentHeight returning 0 for abnormally deep graphs.
High
H1: Mongoose insertMany type assertion masks runtime failures
src/lib/models/Triple.ts:84
const result = await (model.insertMany(batch, { ordered: false }) as Promise<unknown>).then(Casting to Promise<unknown> bypasses TypeScript's type checking entirely. Mongoose's Model.insertMany() returns Promise<Document[]> normally, but when called with a callback signature can return void. If Mongoose's type resolution changes in a future version (e.g., due to upgraded @typegoose/typegoose), this cast would silently pass compilation while .then() could fail at runtime.
Recommendation: Use a proper type annotation and catch rejection via try/catch rather than .then() chains with casts:
try {
await model.insertMany(batch, { ordered: false });
} catch (err) {
// handle error
}H2: TF-IDF numSeeds fallback computes wrong denominator
src/lib/predicates/metrics.ts:42–44
const effectiveNumSeeds =
numSeeds ??
seedCoverages.reduce((max, { countSubj, countObj }) => Math.max(max, countSubj, countObj), 0);The fallback computes max(countSubj, countObj) across all seed coverages — which is the maximum per-predicate coverage count, not the number of seeds. These are semantically different values, and using the wrong one would silently inflate all TF-IDF scores. The only caller (computeOnce at line 121) always passes allSeeds.size, so this fallback is dead code that creates a latent bug for future callers.
Recommendation: Make numSeeds a required parameter. If the fallback is kept, log a warning when exercised.
H3: Missing composite index on PredicatePair causes collection scan
src/lib/predicates/metrics.ts:231
await PredicatePairModel.deleteMany({ sgid, stepIndex });Without a compound index on (sgid, stepIndex), each deleteMany triggers a full collection scan. For subgraphs with many steps and large pair tables (removing the TOP_N limit means potentially thousands of pairs), this adds unnecessary latency.
Recommendation: Add @index({ sgid: 1, stepIndex: 1 }) to PredicatePairClass in src/lib/models/SubGraph.ts.
H4: computePredicateTfIdfScores silently skips predicates in seedCoverages not in counts
src/lib/predicates/metrics.ts:62
The loop iterates countMap.keys() (predicates from predicateCounts). A predicate in seedCoverages or predicateDistincts but missing from predicateCounts (0 triples with positive coverage — contradictory but possible from inconsistent data) is silently ignored. Adding a log.warn would help surface data inconsistencies.
Medium
M1: Hardcoded BATCH_LINES = 5000 should be configurable
src/routes/api/subgraphs/[sgid]/update-crawling-results/+server.ts:9
Optimal batch size depends on triple size (a triple with a long literal is much larger than a short IRI) and available memory. Extract to src/lib/predicates/constants.ts or an env var.
M2: getSeedPredicateCoverage SPARQL query grows linearly with seed count
src/lib/fuseki/fuseki-client.ts:186–199
VALUES ?subjSeed { <seed1> <seed2> ... <seedN> }With large seed sets, this produces very long SPARQL query strings. Consider chunking seeds or using a temporary named graph.
M3: Batching in upsertMany doesn't account for BSON document size limit
src/lib/models/Triple.ts:82–84
1000 triples per batch could exceed MongoDB's 16MB BSON limit if triples contain large literals. Consider size-based splitting as a fallback.
M4: parseCsv strips line-level whitespace but not per-column whitespace
src/lib/predicates/metrics.ts:258
.flatMap((line) => mapFn(line.trim().split(',')))line.trim() strips leading/trailing whitespace from the entire line but individual column values are not trimmed. SPARQL CSV shouldn't have leading/trailing spaces in values, but if they occur, they'd pass through as-is while trailing whitespace before a comma is removed by trim(). Split first, then trim each column value.
M5: Logger test accesses consoleOutput[0] without checking length first
src/lib/logger.spec.ts:55, 58, 63, 69
Several test cases access consoleOutput[0] directly. If the logger fails silently, .toContain() on undefined throws a less helpful error. Should add expect(consoleOutput.length).toBe(1) before asserting content in the test:splat, test:obj, and test:noarg tests.
M6: computeLCRAndAvgHeight deduplicates edges directionally — LCR is undirected but adjacency is directed
src/lib/predicates/metrics.ts:410–414
The seenEdges set is keyed on ${s}|${o}, so a->b and b->a are treated as different edges. DSU treats both the same (undirected union), but adjacency treats them as separate directed edges. This affects computeComponentHeight since it uses the directed adjacency for longest-path computation. This is likely desired but should be documented in the JSDoc.
Low
L1: Dead singleton caching code in fuseki-client.ts
src/lib/fuseki/fuseki-client.ts:11–13 (pre-existing)
The module-level cached variable is read at import time before any createClient() call. The caching inside createClient() uses global.fuseki which works, but the module-level initialization is dead code.
L2: entryPredTfIdf optional in schema but required in interface
src/lib/models/SubGraph.ts:240 vs src/lib/predicates/metrics.ts:382
Schema marks entryPredTfIdf as required: false / optional (?), but the PredicatePair interface declares it as entryPredTfIdf: number (required). Either make the schema required or the interface optional.
L3: No error logged for unrecognized webhook event types
src/lib/webhook/index.ts:13–34
Unrecognized ev.messageType values silently fall through the switch. A log.warn() default case would help debug integration issues.
L4: Array.reduce() on empty seedCoverages throws when numSeeds is undefined
src/lib/predicates/metrics.ts:43
reduce() on an empty array without an initial value throws TypeError. The only caller always passes numSeeds, but the API says numSeeds? is optional. Either make it required or add an initial value of 0.
Style / Convention
S1: Import ordering in predicates/metrics.ts
Lines 1–4 import from $lib/derzis, $lib/fuseki, $lib/models. Line 6 imports from $lib/predicates/constants. Line 12 imports from $lib/logger — these are interspersed with local constants (MAX_RETRIES, etc.). Per AGENTS.md, group $lib imports before local constants.
S2: const BATCH_SIZE = 1000 defined inside static method body (Triple.ts:76)
Consider sharing from a constants module if this value is also relevant elsewhere.
Tests
Strengths:
constants.test.tsverifies all 6 coefficient values — quick regression guard- TF-IDF edge cases: empty input, zero division, subj vs obj selection, predicates in seedCoverages not in counts
- Multi-path DAG height tests directly verify the scenario motivating the Kahn's DP rewrite
- Logger tests cover module names, level labels, timestamps, splat args, objects
Gaps:
- No unit test for
computeComponentHeightin isolation (unexported but tested indirectly viacomputeLCRAndAvgHeight) - No unit test for
upsertManyMongoose method - No parser test for the new CSV column parsing functions (
predColsToDistincts,seedCovColsToObj) - Logger tests don't exercise the custom level edges (
silly,verbose)
Summary
| Severity | Count | Key items |
|---|---|---|
| Critical | 1 | Stack overflow from recursive Tarjan's SCC |
| High | 4 | Type cast in insertMany, TF-IDF fallback bug, missing index, silent skip |
| Medium | 6 | Hardcoded batch size, SPARQL growth, BSON limit, test robustness |
| Low | 4 | Dead code, schema/interface mismatch, missing warning |
The PR is well-architected and improves correctness and performance. The highest-impact items to address before merging are C1 (stack overflow risk) and H1 (type cast that masks runtime errors).
…oid unsafe type assertion
…remove unsafe fallback
…at/obj/noarg tests
…teLCRAndAvgHeight
|
All 11 issues from the second review have been addressed across 8 new commits (plus 1 production bug fix): C1 (iterative SCC): Replaced recursive Tarjan with iterative Kosaraju Plus one production bug: stripped MongoDB ObjectId from predicate pairs via 284 tests pass, lint clean. Ready for re-review. |
andrefs
left a comment
andrefs
left a comment
There was a problem hiding this comment.
Adversarial Code Review — PR #31
I have reviewed all ~25 commits. Below are the issues found, ordered by severity.
🔴 CRITICAL
1. SPARQL injection vectors in existing Fuseki client methods (not fixed by this PR)
src/lib/fuseki/fuseki-client.ts:109 — getSeedAdjacentPredMetrics interpolates seed values directly without escapeURI:
seeds.map((s) => `<${s}>`).join(' ')Same issue in getSeedInInteractions (line 235) and getSeedOutInteractions (line 216). This PR introduced escapeURI and uses it in the new methods (getPredicateDistinctResources, getSeedPredicateCoverage) but did NOT backport it to the existing methods. A seed URI containing > or a closing angle bracket would break the SPARQL syntax. Since seeds come from user-configurable step parameters, this is exploitable.
The fix: apply escapeURI to ALL seed interpolation sites in fuseki-client.ts, not just the new ones.
2. XSS via {@html} in score tooltip — latent injection sink
src/lib/components/subgraph/SubGraphStepDetails.svelte:537-538
<!-- eslint-disable-next-line svelte/no-at-html-tags -->
{@html row.scoreTooltip}The scoreTooltip is built by string concatenation in pairsRows (line 123-144). While the current construction only uses numeric values (safe), this is a ticking XSS bomb. The eslint-disable-next-line comment suppresses the linter warning. Predicate IRIs (which ARE rendered nearby in the table) are only one refactor away from being added to the tooltip. If a malicious IRI containing <script>alert(1)</script> ever ends up in the triple store and gets included in the tooltip HTML, it will execute unsanitized.
The fix: either use {row.scoreTooltip} (Svelte auto-escapes), or sanitize via sanitize-html/DOMPurify if HTML tooltips are truly required.
3. Logger formatArgs eagerly evaluates util.inspect on every call — confirmed regression
src/lib/logger.ts:73-75
const formatArgs = (msg: string, ...args: unknown[]) => {
if (args.length === 0) return msg;
return msg + ' ' + args.map((a) => util.inspect(a, { colors: true })).join(' ');
};This is called on every log line regardless of whether the level is active. A log.debug(hugeObject) will always run util.inspect, even when LOG_LEVEL=info. The PR attempted a lazy approach (commit 4adba5db) and reverted it (commit 1d418412) claiming "pino v8 does not support function-based messages." This is not true — pino v8 does support the err-style childFn({ msg: () => string }) pattern via the genLog mechanism. The revert was premature. For production deployments where debug/silly levels are suppressed, this is a measurable CPU cost on every hot-path log site.
The fix: either use pino's native splat formatting (child.info(msg, arg1, arg2)) or wrap the message in a getter so pino only evaluates when needed.
🟠 HIGH
4. batchInsert error type-casting is fragile across MongoDB driver versions
src/lib/models/Triple.ts:88-89
const mongoErr = err as Error & {
code?: number;
writeErrors?: Array<{ code: number }>;
};Mongoose insertMany({ ordered: false }) errors may throw MongoBulkWriteError (v6+) or BulkWriteError (v5). The shape of writeErrors differs between versions. If a future Mongoose update changes the error structure, the .writeErrors.some() check (line 93) could silently:
- Swallow real non-duplicate errors (if
writeErrorsis absent butcodeis 11000), or - Re-throw recoverable duplicate-key errors (if
writeErrorsis present but the top-levelcodeis not 11000).
Consider using mongoose.Error discriminators or checking err.name/err[Symbol.for('mongoose#error')] instead of brittle type assertions.
5. remove TOP_N limit without pagination — OOM risk
src/lib/predicates/metrics.ts:235-245 — all pairs are stored (previously limited to 100). For a subgraph with 200 entry predicates × 500 hierarchical predicates = 100,000 pairs, insertMany with 100K documents in a single call could exceed MongoDB's 16 MB document limit (each document is ~300+ bytes with IRIs), and the pairs array in memory is equally large. The batchInsert pattern is already used in Triple.ts (1000-item batches) — apply the same pattern here.
6. PredicatePair.deleteMany({ sgid, stepIndex }) + insertMany is not atomic
src/lib/predicates/metrics.ts:235-236: A crash between deleteMany and insertMany leaves the step with zero pairs. On retry, computeOnce will re-insert them, but in the window between crash and retry, the web UI shows an empty pairs table. Use bulkWrite with replaceOne({ filter: ... }, ..., { upsert: true }) to make this atomic.
7. postDerzisBranchFactors response shape is unchecked
src/lib/predicates/metrics.ts:185
const bfResp = await postDerzisBranchFactors(sg.derzis.pid, hierPreds);
for (const r of bfResp.data.results) { ... }If the API returns an error response or data.results is not iterable (e.g., null, undefined, object), this throws TypeError: bfResp.data.results is not iterable. The error is caught by the retry loop, but the message is opaque. Add a type guard or zod schema validation.
🟡 MEDIUM
8. downloadCsv races with URL.revokeObjectURL
src/lib/components/subgraph/SubGraphStepDetails.svelte:233-234
a.click();
URL.revokeObjectURL(url);a.click() is synchronous, but the browser's download manager may not have started fetching the blob URL before revokeObjectURL runs. On Chromium with certain download configurations, this causes the download to fail silently. Use setTimeout(() => URL.revokeObjectURL(url), 1000) or the fetch+blob: download pattern instead.
9. parseCsv column trimming silently corrupts IRI data
src/lib/predicates/metrics.ts:267
.map((s) => s.trim())Trimming every column value is a behavioral change from the previous implementation. If Fuseki ever returns a CSV with quoted values containing leading/trailing whitespace (e.g., " <http://example.org/pred> "), the trimming would strip the inner quotes and produce a malformed IRI. The original code did NOT trim values. Either:
- Skip trimming for predicate IRI columns, or
- Add a warning log when trimming actually changes a value.
10. sanitize function mutates objects in-place and has incorrect Date detection for arrays
src/lib/utils/utils.ts:38-54 — sanitize is now called on _pairs in +page.server.ts:2630. The function:
- Mutates the argument in-place (
delete obj._id) - Recurses into arrays (incorrectly treating numeric indices as object keys)
- Uses
.includes('Date')which for arrays returns[object Array](does NOT include 'Date'), so arrays are recursed into - For Date objects:
Object.prototype.toString.call(new Date())returns[object Date]— this check works, but only because'Date'happens to match. A more precise check would bevalue instanceof Date.
The mutation is probably harmless here (lean docs), but a defensive delete on a potentially frozen/sealed object would throw.
11. getExtendedSeeds SPARQL injection via predicatePattern
src/lib/fuseki/fuseki-client.ts:86-89
objects.map((obj) => `\t{ ?sub ${predicatePattern} <${obj}> . }\n`).join('UNION\n')The predicatePattern parameter is an arbitrary string interpolated directly into the SPARQL query. This was pre-existing but missed in the escapeURI audit. The parameter is used in getExtendedSeeds only, which may accept caller-controlled input.
12. Object.entries(levels) order dependency
src/lib/logger.ts:19
const levelNames = Object.fromEntries(Object.entries(levels).map(([k, v]) => [v, k]));Object.entries on a numeric-valued object yields entries in insertion order, which for the literal object { error: 50, warn: 40, ... } happens to be the definition order. This is technically an implementation detail — if the object were modified at runtime, the order is not guaranteed. Consider using a numeric array [50, 'error', 40, 'warn', ...] and building a proper lookup table.
13. Empty effectiveNumSeeds variable
src/lib/predicates/metrics.ts:42
const effectiveNumSeeds = numSeeds;This variable is assigned but never transformed before use (line 79). It's dead code that obscures intent. Remove it and use numSeeds directly.
🔵 LOW (Nitpicks / Code Quality)
14. computePredicateTfIdfScores unused countMap values
The countMap stores predicate counts that are only used to compute totalTriples (line 41) and for iteration keys. The per-predicate count values are never used individually within the loop. Consider using a Set for iteration if counts aren't needed.
15. Memory: computeComponentHeight builds revAdj as a separate reverse graph
src/lib/predicates/metrics.ts:498-509 — computeSCCs builds revAdj which duplicates the adjacency memory. For large components (10K+ nodes), this doubles memory. Kosaraju's algorithm inherently requires this, but the existing code claims to be a Kosaraju replacement for Tarjan (commit ccaf1e4e — "replace recursive Tarjan SCC with iterative Kosaraju"). This is a known space/time tradeoff, but worth documenting.
16. computeComponentHeight single-SCC fallback returns 0 for all cycles — no differentiation
A 2-node cycle (a→b, b→a) and a 1000-node cycle both get height 0. This is intentional per the comments, but it means the avgHeight metric cannot distinguish between small and large cycles. If this matters for scoring, consider returning the cycle's diameter instead of 0.
17. MonkeyPatchedLogger bypasses pino's structured logging entirely
All log arguments are concatenated into a flat string via formatArgs before reaching pino. This means pino's JSON structured logging, splat formatting, and error object serialization are completely neutered. The custom transport also re-parses the JSON back into a string (line 39: JSON.parse(chunk.toString()) after formatArgs already serialized everything). This round-trips data through stringification twice. A simpler approach would be to pass args as pino's splat and let the transport do the formatting.
18. SubGraphStepDetails.svelte sorts all rows but only displays 100
Line 186: let displayRows = $derived(pairsRows.slice(0, DISPLAY_LIMIT)); — the full pairsRows array is computed (including sorting all pairs) even though only 100 are displayed. For 100K pairs, this sorts 100K elements to display 100. Consider limiting to DISPLAY_LIMIT before sorting via a top-K selection.
✅ What was done well
- The N3 parser async fix in
update-crawling-results/+server.ts(batching + proper Promise wrapping) was a real bug, correctly fixed. - Edge deduplication in
computeLCRAndAvgHeightviaseenEdgesis correct and well-tested. - The
insertMany({ ordered: false })+ write-error filtering is a significant performance improvement over the old per-tripleexists()pattern. - Migrating scoring constants into a shared
$lib/predicates/constants.tseliminates a real source of drift. - Adding
sanitizeto the page load output correctly prevents MongoDB ObjectId serialization errors.
Summary of missed issues from previous reviews
Previous reviews flagged:
- Logger lazy evaluation — was attempted, reverted, and remains broken (issue #3 above).
- SCORE_D=0 makes LCR term dead in scoring — acknowledged in PR body, but the code still computes and stores LCR. If the coefficient might be re-enabled, document that in a comment. Currently it's wasted Fuseki queries for every hierarchical predicate.
- Triple import OOM — fixed via batching (good).
- Atomicity of pair replacement — NOT addressed (issue #6 above).
1 participant
Summary
Overhaul of the predicate pair scoring system with TF-IDF entry predicate scoring and average component height metrics. Refactored logging from winston to pino for structured logging. Fixed critical bugs in triple import (N3 async parser) and MultiSelectInput (duplicate key error). Performance improvements to Triple.upsertMany.
Key Changes
Scoring system overhaul
TF-IDF entry predicate scoring (
src/lib/predicates/metrics.ts): NewcomputePredicateTfIdffunction using Fuseki SPARQL queries for predicate counts, distinct resources, and seed coverage. Integrated as SCORE_E = 1 in the pair score formula.Average component height metric:
computeLCRAndAvgHeightreplacescomputeLCR, returning both LCR and the average height of components for hierarchical predicates. Integrated as SCORE_F = 1.2.Scoring coefficient adjustments:
Removed the top-100 pairs limit — all predicate pairs are now stored and exported.
Pair score tooltips (
src/lib/components/subgraph/SubGraphStepDetails.svelte)Logger refactoring: winston → pino
winstonwithpinofor structured logging across all server-side modules.colorettefor colored console output with custom level-to-color mapping.createLoggerimports across all files (was importing fromvite, now from$lib/logger).logger.spec.tsunit tests.pino8.21.0 +colorette2.0.20, removedwinston3.17.0.Triple import & storage fixes
src/lib/models/Triple.ts: RewroteupsertManyto use batchinsertMany({ ordered: false })instead of per-tripleexists()checks (eliminates O(n) DB round-trips). Handles deduplication via MongoDB unique index, ignoring error code 11000.src/routes/api/subgraphs/[sgid]/update-crawling-results/+server.ts: Fixed critical N3 Parser async bug. The N3 Parser operates asynchronously when a callback is supplied —_lexer.tokenize()schedules callbacks via microtasks. The previous code checkedbatch.lengthimmediately afterparse()returned, before any callbacks fired, yielding empty batches and 0 imported triples. Now each batch'sparse()call is wrapped in a Promise that resolves on the null-quad completion signal. Also processes N-Triples in 5000-line chunks for memory efficiency.src/lib/components/MultiSelectInput.svelte: Deduplicate theoptionsprop via[...new Set(options)]to prevent Svelteeach_key_duplicateerror when the same IRI appears multiple times in the dropdown (e.g., rdfs:seeAlso at two indices).Fuseki client (src/lib/fuseki/fuseki-client.ts)
getPredicateDistinctResources: SPARQL query returning distinct subject/object counts per predicate.getSeedPredicateCoverage: SPARQL query returning per-predicate seed coverage (seeds-as-subject vs seeds-as-object).SubGraph model (src/lib/models/SubGraph.ts)
avgHeightHierPredonPredicatePairClassfor storing average component height metrics.Cleanup
src/routes/subgraphs/[sgid]/+page.server.ts: Removed duplicateupdateCrawlingResultsform action. The UI button in+page.sveltealready calls the standalone API endpoint via fetch; the form action was dead code.Environment/Tooling
package.json: AddedNODE_OPTIONS='--max-old-space-size=8192'to the dev script to prevent OOM during large graph processing.package-lock.jsonfor new/removed dependencies.Files Changed
src/lib/predicates/metrics.tssrc/lib/components/subgraph/SubGraphStepDetails.sveltesrc/lib/logger.tssrc/lib/logger.spec.tssrc/lib/models/Triple.tssrc/lib/models/SubGraph.tssrc/lib/fuseki/fuseki-client.tssrc/routes/api/subgraphs/[sgid]/update-crawling-results/+server.tssrc/lib/components/MultiSelectInput.sveltesrc/routes/subgraphs/[sgid]/+page.server.tssrc/lib/derzis/derzis-api.tssrc/lib/derzis/metrics.tssrc/lib/fuseki/fuseki-api.tssrc/lib/label-service.tssrc/lib/webhook/*.ts(6 files)src/routes/api/subgraphs/[sgid]/*.ts(4 files)src/routes/subgraphs/+page.server.tspackage.jsonpackage-lock.jsonBreaking / Migration Notes
createLoggerfromvitenow import from$lib/logger. Verify any custom server-side code outside this repo uses the new import path.