Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"test:watch": "vitest",
"test:coverage": "vitest run --coverage",
"test:e2e": "playwright test",
"test:e2e:ci": "npm run build && PLAYWRIGHT_MODE=production playwright test",
"typecheck": "tsc --noEmit",
"cli": "node --loader ts-node/esm cli/index.ts"
},
Expand Down
15 changes: 9 additions & 6 deletions src/db/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
import { AppError } from '../lib/errors.js';
import { ConnectionPool, DEFAULT_POOL_SIZE } from './connection-pool.js';
import sqlite3InitModule from '@sqlite.org/sqlite-wasm';
import * as fs from 'fs';

export interface SQLiteDB {
exec: (options: string | {
Expand Down Expand Up @@ -31,12 +30,16 @@
const schemaResponse = await fetch('/db/schema.sql');
if (schemaResponse.ok) return await schemaResponse.text();
}
// Fallback to local fs for CLI
try {
return fs.readFileSync('./public/db/schema.sql', 'utf-8');
} catch {
return '';
// Fallback for CLI
if (typeof process !== 'undefined' && process.versions && process.versions.node) {

Check warning on line 34 in src/db/client.ts

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

src/db/client.ts#L34

Unnecessary conditional, value is always truthy.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM RISK

The environment check for Node.js is flagged as redundant. You can simplify this using optional chaining, which is safer and satisfies the linter's requirement for non-constant conditionals in a multi-environment codebase.

Suggested change
if (typeof process !== 'undefined' && process.versions && process.versions.node) {
if (typeof process !== 'undefined' && process.versions?.node) {

See Issue in Codacy

try {
const fs = await import('fs');
return fs.readFileSync('./public/db/schema.sql', 'utf-8');
} catch {
return '';
}
}
return '';
};

const isBrowser = typeof window !== 'undefined' && typeof Worker !== 'undefined';
Expand Down
15 changes: 15 additions & 0 deletions src/db/repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,21 @@ export class Repository {
}
}

async getAllClaims(): Promise<Claim[]> {
try {
const results = await this.db.exec({
sql: `SELECT * FROM claims`,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Template string can be replaced with regular string literal


Template literals are useful when you need: 1. Interpolated strings.

returnValue: 'resultRows',
rowMode: 'object',
});
const rows = z.array(z.unknown()).parse(results);
return rows.map((r) => this.parseMetadata(ClaimSchema, r));
} catch (err) {
logger.error('Failed to fetch all claims', err);
throw new AppError('Failed to fetch all claims', 'DB_ERROR', err);
}
}

async updateClaim(id: string, claim: Partial<Claim>): Promise<Claim> {
try {
const results = await this.db.exec({
Expand Down
1 change: 1 addition & 0 deletions src/lib/__tests__/search.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ vi.mock('@orama/orama', () => ({
vi.mock('../../db/repository', () => ({
repository: {
getAllEntities: vi.fn().mockResolvedValue([]),
getAllClaims: vi.fn().mockResolvedValue([]),
getEntityById: vi.fn().mockResolvedValue(null),
getClaimsByEntityId: vi.fn().mockResolvedValue([]),
},
Expand Down
7 changes: 5 additions & 2 deletions src/lib/__tests__/search_init_perf.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ describe('Search Initialization Benchmark', () => {
vi.clearAllMocks();
});

it('measures initSearch performance with 100 entities and 500 claims', async () => {
const numEntities = 100;
it('measures initSearch performance with 1000 entities and 5000 claims', async () => {
const numEntities = 1000;
const claimsPerEntity = 5;

const entities = Array.from({ length: numEntities }, (_, i) => ({
Expand Down Expand Up @@ -58,5 +58,8 @@ describe('Search Initialization Benchmark', () => {

console.log(`initSearch took ${end - start}ms`);
expect(repository.getAllEntities).toHaveBeenCalledTimes(1);
expect(repository.getAllClaims).toHaveBeenCalledTimes(1);
// Should NOT call these during bulk init anymore
expect(repository.getEntityById).toHaveBeenCalledTimes(0);
});
});
59 changes: 47 additions & 12 deletions src/lib/search.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { create, insert, remove, search, type Orama } from '@orama/orama';
import { create, insert, insertMultiple, remove, search, type Orama } from '@orama/orama';
import { repository } from '../db/repository.js';
import { logger } from './logger.js';
import { compressText } from './nlp.js';
Expand All @@ -13,14 +13,14 @@ interface SearchDocument {
keywords: string;
}

type OramaSchema = typeof searchSchema;
const searchSchema = {
id: 'string',
type: 'string',
title: 'string',
content: 'string',
keywords: 'string',
} as const;
export type OramaSchema = typeof searchSchema;

let oramaDb: Orama<OramaSchema> | null = null;
const oramaIdMap = new Map<string, string>(); // entityId → oramaInternalId
Expand Down Expand Up @@ -68,21 +68,56 @@ export const initSearch = async () => {

try {
oramaDb = await create({
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM RISK

oramaDb is assigned before documents are inserted. This can lead to race conditions where other parts of the application attempt to search an empty index while it is still being populated. Perform the initialization using a local variable and assign it to the top-level oramaDb only after insertMultiple succeeds.

Try running the following prompt in your coding agent:

Refactor initSearch in src/lib/search.ts to use a local variable for the Orama instance during initialization, and only assign it to the module-level oramaDb variable at the end of the function.

schema: {
id: 'string',
type: 'string',
title: 'string',
content: 'string',
keywords: 'string',
},
schema: searchSchema,
});

const entities = await repository.getAllEntities();
const [entities, allClaims] = await Promise.all([
repository.getAllEntities(),
repository.getAllClaims(),
]);

// Group claims by entity_id for efficient lookup
const claimsByEntity = new Map<string, Claim[]>();
for (const claim of allClaims) {
const list = claimsByEntity.get(claim.entity_id) || [];
list.push(claim);
claimsByEntity.set(claim.entity_id, list);
}

const docs: SearchDocument[] = [];
const originalIds: string[] = [];

for (const entity of entities) {
await indexEntityById(entity.id!);
docs.push({
id: entity.id!,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forbidden non-null assertion


Using non-null assertions cancels out the benefits of strict null-checking, and introduces the possibility of runtime errors. Avoid non-null assertions unless absolutely necessary. If you still need to use one, write a skipcq comment to explain why it is safe.

type: 'entity',
title: entity.name,
content: compressText(`${entity.name} ${entity.description || ''}`),
keywords: entity.type,
});
originalIds.push(entity.id!);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forbidden non-null assertion


Using non-null assertions cancels out the benefits of strict null-checking, and introduces the possibility of runtime errors. Avoid non-null assertions unless absolutely necessary. If you still need to use one, write a skipcq comment to explain why it is safe.


const claims = claimsByEntity.get(entity.id!) || [];
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forbidden non-null assertion


Using non-null assertions cancels out the benefits of strict null-checking, and introduces the possibility of runtime errors. Avoid non-null assertions unless absolutely necessary. If you still need to use one, write a skipcq comment to explain why it is safe.

for (const claim of claims) {
docs.push({
id: claim.id!,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forbidden non-null assertion


Using non-null assertions cancels out the benefits of strict null-checking, and introduces the possibility of runtime errors. Avoid non-null assertions unless absolutely necessary. If you still need to use one, write a skipcq comment to explain why it is safe.

type: 'claim',
title: entity.name,
content: compressText(claim.statement),
keywords: [entity.id!, claim.source || 'unknown'].join(','),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forbidden non-null assertion


Using non-null assertions cancels out the benefits of strict null-checking, and introduces the possibility of runtime errors. Avoid non-null assertions unless absolutely necessary. If you still need to use one, write a skipcq comment to explain why it is safe.

});
originalIds.push(claim.id!);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forbidden non-null assertion


Using non-null assertions cancels out the benefits of strict null-checking, and introduces the possibility of runtime errors. Avoid non-null assertions unless absolutely necessary. If you still need to use one, write a skipcq comment to explain why it is safe.

}
Comment on lines +91 to +110
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM RISK

Suggestion: The mapping logic for transforming entities and claims into search documents is now duplicated between the bulk 'initSearch' and the single-item 'addEntityToIndex'. This creates a maintainability risk where the two indexing paths could diverge.

Try running the following prompt in your IDE agent:

Refactor src/lib/search.ts to extract two private helper functions: 'mapEntityToSearchDoc(entity: Entity): SearchDocument' and 'mapClaimToSearchDoc(claim: Claim, entityName: string): SearchDocument'. Update both 'addEntityToIndex' and 'initSearch' to use these helpers to ensure consistent indexing logic across the application.

}

if (docs.length > 0) {
const oramaIds = await insertMultiple(oramaDb, docs);
for (let i = 0; i < originalIds.length; i++) {
oramaIdMap.set(originalIds[i], oramaIds[i]);
}
}

logger.info('Orama search index initialized');
logger.info(`Orama search index initialized with ${entities.length} entities and ${allClaims.length} claims`);

// Register job handlers
jobCoordinator.registerHandler('reindex-document', async (payload) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH RISK

The job handlers are registered inside initSearch. Since the refresh-search-index handler calls initSearch, new handlers are added every time the index is refreshed. Move these registrations outside the function to ensure they are only registered once.

Try running the following prompt in your coding agent:

Move the jobCoordinator.registerHandler calls in src/lib/search.ts outside of the initSearch function, ensuring they are only called once during module initialization.

Expand Down
Loading