SF-19-in-memory-singleton-for-config-data#12
Conversation
Eliminates legacy configuration utilities and their exports to streamline the config resolution logic. Updates tests to use the recommended config accessor, improving maintainability and reducing deprecated code paths.
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a comprehensive overhaul of the logging, error handling, and configuration systems in the flow-service and flow-core modules. It adds a new structured, multi-channel logging framework with support for file and Sentry logging, enhanced error handling with contextual metadata, and a modularized configuration system leveraging Quadstore for graph-based config storage and merging. Numerous new utilities, types, and test suites are introduced, while legacy logger and test scripts are removed or replaced. Changes
Sequence Diagram(s)sequenceDiagram
participant ServiceModule as Service Module (e.g., Route, Config)
participant Logger as StructuredLogger
participant FileLogger as FileLogger
participant Sentry as Sentry
participant ErrorHandler as Error Handler
ServiceModule->>Logger: log(level, message, context)
Logger->>FileLogger: writeToFile(formattedLog)
Logger->>Sentry: reportErrorToSentry(error, level, context)
ServiceModule->>ErrorHandler: handleCaughtError(error, message, context)
ErrorHandler->>Logger: error(level, message, context)
ErrorHandler->>Sentry: reportErrorToSentry(error, level, context)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🔭 Outside diff range comments (3)
flow-service/tests/integration/logging/sentry-logging.test.ts (1)
1-7: Placeholder test will currently breakno-unused-locals/no-unused-varslint rulesBoth
loggerandconfigare imported but never used.
Iftsconfig.jsonhasnoUnusedLocals(common in this repo) the test suite will not compile.Two quick fixes:
-import { logger } from '../../../src/utils/logger.ts'; -import { singletonServiceConfigAccessor as config } from '../../../src/config/resolution/service-config-accessor.ts'; +// TODO: remove once the first assertions are added +// import { logger } from '../../../src/utils/logger.ts'; +// import { singletonServiceConfigAccessor as config } from '../../../src/config/resolution/service-config-accessor.ts';…or stub a minimal test:
Deno.test('sentry-logging – placeholder', () => { logger.info('placeholder'); // prevents unused-import error });Until real assertions land, consider
Deno.test({ name, ignore: true, fn })to keep CI green.flow-service/tests/integration/logging/log-retention-config.test.ts (1)
1-6: Imports are unused and the file contains noDeno.testblocksThis will fail under
noUnusedLocals/noUnusedVars.
Either mark the file as ignored for now or add a stub test that at least exercisesloadEnvConfigso the imports are used.flow-service/src/services/metadata-composer.ts (1)
25-26: Replace synchronous config property access with async gettersThe
ServiceConfigAccessoronly exposes async methods (getPort(),getHost(),getMeshPaths()), soconfig.defaultAttributedToandconfig.defaultDelegationChainwill always beundefined. You should either:
Implement async getters in
ServiceConfigAccessorfordefaultAttributedToanddefaultDelegationChainthat run the appropriate SPARQL queries, e.g.:async getDefaultAttributedTo(): Promise<AttributedTo | undefined> { … } async getDefaultDelegationChain(): Promise<DelegationChain | undefined> { … }Or load those values from a synchronous source before composing metadata.
Then update
composeMetadataContentto beasyncand use:- const attributedTo = config.defaultAttributedTo; - const delegationChain = config.defaultDelegationChain; + const attributedTo = await config.getDefaultAttributedTo(); + const delegationChain = await config.getDefaultDelegationChain();Files to update:
flow-service/src/config/resolution/service-config-accessor.ts– add the two async getters.flow-service/src/services/metadata-composer.ts– makecomposeMetadataContentasync and replace the two property accesses withawaitcalls.
♻️ Duplicate comments (1)
flow-service/tests/integration/logging/sentry-init.test.ts (1)
1-8: Same unused-import issue as the previous file
initSentry,logger, andconfigare imported but never referenced.
Apply one of the mitigation approaches described in the previous comment to avoid TypeScript compile failures in CI.
🧹 Nitpick comments (28)
.gitignore (1)
12-12: Consider using a directory-specific ignore pattern
logswill ignore both a sibling file calledlogsand any directory namedlogsat any depth.
If the intent is only to ignore the directory, a trailing slash makes that explicit and avoids accidental skips of legitimately-named files:-logs +logs/package.json (1)
1-5: Add the corresponding ESLint plugin for TypeScript rulesOnly the parser is listed; most TS-specific rules live in
@typescript-eslint/eslint-plugin.
Without it, your config will warn that rules are unknown or silently do nothing."devDependencies": { "@typescript-eslint/parser": "^8.38.0", + "@typescript-eslint/eslint-plugin": "^8.38.0", "eslint": "^9.32.0" }Ensure
eslint.config.mjs(or equivalent) extends the plugin to activate the rules..ai-guidance.md (1)
17-20: Minor wording/grammar tweak for clarity“Too-long files are cause syntax errors” reads awkwardly. Suggested rephrase:
-So important! Too-long files are cause syntax errors from agent edits. +So important! Overly long files often trigger syntax errors during automated agent edits.deno.json (1)
14-16: Task whitelist looks correct but double-space after--allow-readis harmlessNothing functionally wrong; feel free to trim the extra space next time.
flow-service/tests/integration/logging/handle-caught-error.test.ts (2)
8-15: Deep relative import paths are brittle – recommend using the import-mapYou’ve updated the paths to
../../../src/..., which works today but will break on the next directory move.
Since Deno already supports import maps (and this repo ships one for@deps/*), add an entry:{ "imports": { "@flow/logger": "./src/utils/logger.ts", "@flow/errors": "./src/utils/errors.ts", "@flow/config-types": "./src/config/config-types.ts" } }Then the imports become:
import { handleCaughtError } from '@flow/logger'; import { ConfigurationError, FlowServiceError, ValidationError } from '@flow/errors'; import { ConfigValidationError } from '@flow/config-types';Result: shorter lines, no
../../../gymnastics, and easier refactors.
90-100: Minor nit – test numbering skips 9Lines 90-100 label the section “Test 10”, but there is no “Test 9”.
Won’t break anything, yet can confuse readers scanning logs.flow-service/src/utils/logger.ts (1)
221-259: ExportingformatConsoleMessageis 👍 but mind ANSI padding
prefixalready right-pads the raw level.
Inside theisDevelopmentbranch you docoloredLevel.padEnd(5)again, but the escape codes inflate the length, so the visual width is off.
Consider padding before coloring:-const coloredLevel = level === 'ERROR' || level === 'CRITICAL' - ? colorize('red', level) +const rawPaddedLevel = level.padEnd(5); +const coloredLevel = level === 'ERROR' || level === 'CRITICAL' + ? colorize('red', rawPaddedLevel) : …; -const coloredPrefix = colorize('gray', `[${timestamp}]`) + - ` ${coloredLevel.padEnd(5)}`; +const coloredPrefix = colorize('gray', `[${timestamp}]`) + + ` ${coloredLevel}`;
- The function is now public – please add a short JSDoc to make its intended, internal-only audience explicit and discourage external callers from bypassing
logger.flow-service/src/quadstore-default-bundle.ts (1)
4-11: Consider using the factory function to reduce code duplication.This code duplicates the logic from
createQuadstoreBundleinquadstore-factory.ts. Consider using the factory function for consistency and maintainability.-const backend = new MemoryLevel(); -const df = new DataFactory(); -const store = new Quadstore({ backend, dataFactory: df }); -const engine = new Engine(store); - -await store.open(); - -export const defaultQuadstoreBundle: QuadstoreBundle = { store, df, backend, engine }; +import { createQuadstoreBundle } from '../../flow-core/src/utils/quadstore/quadstore-factory.ts'; + +export const defaultQuadstoreBundle: QuadstoreBundle = await createQuadstoreBundle();flow-core/src/types.ts (1)
7-8: Consider adding JSDoc documentation.Adding JSDoc comments would improve the interface documentation and developer experience.
-// Quadstore Bundle Type +/** + * A bundle containing all the necessary components for a Quadstore setup. + * @template B - The backend type, extending AbstractLevel + */ export interface QuadstoreBundle<B extends AbstractLevel<any, any, any> = AbstractLevel<any, any, any>> {flow-service/tests/integration/quadstore-accessor.test.ts (2)
37-37: Consider adding null check instead of non-null assertion.The non-null assertion operator (
!) assumes the engine is always present, but it would be safer to add a proper null check for more robust error handling.- const bindingsStream = await engine!.queryBindings(query, { sources: [store] }); + if (!engine) { + throw new Error("Query engine is not available"); + } + const bindingsStream = await engine.queryBindings(query, { sources: [store] });
38-40: Improve type safety for bindings stream iteration.The
as anycast bypasses TypeScript's type checking. Consider using proper typing or adding runtime validation for the binding structure.- for await (const binding of bindingsStream as any) { - result.push(binding.get("o").value); + for await (const binding of bindingsStream) { + const objectBinding = binding.get("o"); + if (objectBinding) { + result.push(objectBinding.value); + } }flow-core/src/utils/rdfjs-utils.ts (1)
6-6: Consider making DataFactory configurable or reusable.While creating a local DataFactory instance works, consider whether this should be configurable or if there's a shared instance that could be reused across the application to ensure consistency.
-const df = new DataFactory(); +// Consider making this configurable or using a shared instance +const df = new DataFactory();eslint.config.mjs (1)
15-15: Consider relaxing the quote-props rule.The
"quote-props": ["error", "always"]rule forces quotes on all object properties, even when not required (e.g.,"foo"instead offoo). This can make code less readable. Consider using"as-needed"instead.- "quote-props": ["error", "always"], + "quote-props": ["error", "as-needed"],flow-service/main.ts (1)
48-49: Consider caching frequently accessed configuration valuesThe host and port values are accessed multiple times during startup. Since these values don't change during runtime, consider caching them after the first access to avoid repeated async calls.
+// Cache frequently accessed config values after initialization +const host = await singletonServiceConfigAccessor.getHost(); +const port = await singletonServiceConfigAccessor.getPort(); + servers: [ { - url: `http://${await singletonServiceConfigAccessor.getHost()}:${await singletonServiceConfigAccessor.getPort()}`, + url: `http://${host}:${port}`, description: 'Configured server', }, ], // ... later in the file Deno.serve({ - port: await singletonServiceConfigAccessor.getPort(), - hostname: await singletonServiceConfigAccessor.getHost(), + port: port, + hostname: host, }, app.fetch);Also applies to: 101-102
flow-service/src/config/resolution/service-config-validator.ts (1)
2-6: Consider extracting predicate URIs as constantsThe hardcoded URIs should be defined as constants for better maintainability and consistency across the codebase.
Add these constants at the module level or import from a shared constants file:
const FSVC_NAMESPACE = 'https://semantic-flow.github.io/ontology/flow-service/'; const FSVC_PORT = `${FSVC_NAMESPACE}port`; const FSVC_HOST = `${FSVC_NAMESPACE}host`;flow-service/tests/integration/logging/formatting-direct.test.ts (1)
146-146: Update the year check in log validationThe hardcoded year check
[2025-might fail in future years. Consider making it more flexible.- if (logContent.includes('[2025-') && !logContent.startsWith('{"timestamp"')) { + const currentYear = new Date().getFullYear(); + if (logContent.includes(`[${currentYear}-`) && !logContent.startsWith('{"timestamp"')) {flow-core/tests/unit/quadstore-utils.test.ts (1)
1-86: Consider extracting common test setupEach test creates its own quadstore bundle. Consider extracting common setup to reduce duplication and improve test performance.
// Add at the top of the file let testQuadstoreBundle: any; let df: any; let store: any; // Setup before all tests Deno.test({ name: "quadstore-utils tests", fn: async (t) => { testQuadstoreBundle = await createQuadstoreBundle(); df = testQuadstoreBundle.df; store = testQuadstoreBundle.store; await t.step("countQuadsInStream counts quads correctly", async () => { // ... existing test logic }); // ... other test steps }, sanitizeResources: false, sanitizeOps: false, });flow-service/tests/integration/quadstore-utils.test.ts (1)
52-55: Use the imported countQuadsInStream directlyThe helper function
countQuadsInGraphis just a thin wrapper that could be inlined to reduce code duplication.Replace the helper function usage with direct calls:
- let count = await countQuadsInGraph(graphA, store); + let count = await countQuadsInStream(store.match(undefined, undefined, undefined, graphA));And remove the helper function definition at lines 52-55.
flow-service/src/utils/startup-logger.ts (1)
42-51: Track incomplete configuration migrationThe TODOs indicate that the configuration migration is incomplete. Several config properties are hardcoded instead of being retrieved from the accessor.
Would you like me to help implement the missing configuration accessor methods or create a GitHub issue to track this technical debt?
flow-service/src/config/resolution/service-config-loader.ts (1)
14-14: Clarify or complete the TODO commentThe TODO comment "dct:conformsTo" is unclear and provides no context about what needs to be implemented.
Please provide more context about this TODO, or would you like me to help research what "dct:conformsTo" refers to in this context and create an issue to track it?
flow-service/src/config/resolution/service-config-accessor.ts (1)
7-85: Consider caching configuration valuesEach config access triggers a SPARQL query. For frequently accessed values like host and port, this could impact performance.
Consider implementing a caching layer:
private cache = new Map<string, { value: any, timestamp: number }>(); private cacheTimeout = 60000; // 1 minute private getCached<T>(key: string, fetcher: () => Promise<T>): Promise<T> { const cached = this.cache.get(key); if (cached && Date.now() - cached.timestamp < this.cacheTimeout) { return Promise.resolve(cached.value); } const value = await fetcher(); this.cache.set(key, { value, timestamp: Date.now() }); return value; }flow-service/src/config/resolution/service-config-resolver.ts (1)
51-53: Address TODO: Implement mesh node config loading.The TODO comment indicates missing functionality for loading mesh root node configuration. This should be implemented to complete the configuration loading pipeline.
Would you like me to help implement the mesh node config loading logic or create an issue to track this task?
flow-service/src/config/config-types.ts (3)
60-72: Complete the TODO: Finalize interface naming.The TODO comment indicates the
MeshRootNodeConfiginterface name might need to be changed. This should be resolved to maintain naming consistency.
88-91: Complete the AttributedTo interface definition.The TODO comment indicates this interface needs a complete definition beyond just the
@idproperty.Would you like me to help define the complete AttributedTo interface based on the ontology or create an issue to track this?
31-36: Consider more compact formatting for the union type.- readonly 'fsvc:logRotationInterval'?: - | 'daily' - | 'weekly' - | 'monthly' - | 'size-based'; + readonly 'fsvc:logRotationInterval'?: 'daily' | 'weekly' | 'monthly' | 'size-based';flow-service/src/config/defaults.ts (2)
12-13: Address TODO: Make local context configurableThe TODO comment indicates that the local context should use the config host and port dynamically. This hardcoded
http://localhost/graph/might not work in all deployment scenarios.Would you like me to help implement a solution that dynamically constructs the local context URL based on the service configuration?
23-24: Consider completing the rename indicated in TODOThe TODO suggests renaming this constant to better indicate its purpose for root MeshNodes. Consider renaming
PLATFORM_NODE_DEFAULTSto something likePLATFORM_MESH_ROOT_NODE_DEFAULTSfor clarity.flow-service/src/config/index.ts (1)
11-18: Consider making graph URIs configurableThe
CONFIG_GRAPH_NAMEScontains hardcodedhttp://localhost/graph/URLs. Similar to the local context in defaults.ts, these should ideally be configurable based on the service's host and port settings.Consider implementing a function that generates these graph URIs dynamically based on the service configuration:
-export const CONFIG_GRAPH_NAMES = { - platformServiceDefaults: 'http://localhost/graph/platformServiceDefaults', - platformImplicitMeshRootNodeConfig: 'http://localhost/graph/platformImplicitMeshRootNodeConfig', - inputServiceConfig: 'http://localhost/graph/inputServiceConfig', - inputMeshRootNodeConfig: 'http://localhost/graph/inputMeshRootNodeConfig', - mergedServiceConfig: 'http://localhost/graph/mergedServiceConfig', - // Per-node effective configs could be stored with dynamic graph names -}; +export function getConfigGraphNames(baseUrl: string = 'http://localhost/graph') { + return { + platformServiceDefaults: `${baseUrl}/platformServiceDefaults`, + platformImplicitMeshRootNodeConfig: `${baseUrl}/platformImplicitMeshRootNodeConfig`, + inputServiceConfig: `${baseUrl}/inputServiceConfig`, + inputMeshRootNodeConfig: `${baseUrl}/inputMeshRootNodeConfig`, + mergedServiceConfig: `${baseUrl}/mergedServiceConfig`, + // Per-node effective configs could be stored with dynamic graph names + }; +} + +export const CONFIG_GRAPH_NAMES = getConfigGraphNames();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
deno.lockis excluded by!**/*.lockflow-service/deno.lockis excluded by!**/*.lockflow-service/logs/test-pretty-direct.logis excluded by!**/*.logpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (45)
.ai-guidance.md(1 hunks).gitignore(1 hunks)deno.json(1 hunks)eslint.config.mjs(1 hunks)flow-core/src/deps.ts(1 hunks)flow-core/src/platform-constants.ts(1 hunks)flow-core/src/types.ts(1 hunks)flow-core/src/utils/quadstore-utils.ts(1 hunks)flow-core/src/utils/quadstore/quadstore-factory.ts(1 hunks)flow-core/src/utils/rdfjs-utils.ts(1 hunks)flow-core/tests/unit/quadstore-utils.test.ts(1 hunks)flow-service/deno.json(1 hunks)flow-service/flow-service-config.jsonld(1 hunks)flow-service/main.ts(5 hunks)flow-service/src/config/config-types.ts(6 hunks)flow-service/src/config/defaults.ts(2 hunks)flow-service/src/config/index.ts(2 hunks)flow-service/src/config/loaders/env-loader.ts(2 hunks)flow-service/src/config/loaders/jsonld-loader.ts(7 hunks)flow-service/src/config/loaders/quadstore-loader.ts(1 hunks)flow-service/src/config/node-config.default.jsonld(1 hunks)flow-service/src/config/resolution/service-config-accessor.ts(1 hunks)flow-service/src/config/resolution/service-config-loader.ts(1 hunks)flow-service/src/config/resolution/service-config-resolver.ts(1 hunks)flow-service/src/config/resolution/service-config-validator.ts(1 hunks)flow-service/src/quadstore-default-bundle.ts(1 hunks)flow-service/src/routes/meshes.ts(2 hunks)flow-service/src/routes/weave.ts(1 hunks)flow-service/src/services/metadata-composer.ts(1 hunks)flow-service/src/utils/logger.ts(1 hunks)flow-service/src/utils/mesh-utils.ts(2 hunks)flow-service/src/utils/startup-logger.ts(3 hunks)flow-service/tests/integration/logging/formatting-direct.test.ts(3 hunks)flow-service/tests/integration/logging/handle-caught-error.test.ts(1 hunks)flow-service/tests/integration/logging/log-retention-config.test.ts(1 hunks)flow-service/tests/integration/logging/logger.test.ts(1 hunks)flow-service/tests/integration/logging/pretty-format-simple.test.ts(1 hunks)flow-service/tests/integration/logging/sentry-init.test.ts(1 hunks)flow-service/tests/integration/logging/sentry-logging.test.ts(1 hunks)flow-service/tests/integration/quadstore-accessor.test.ts(1 hunks)flow-service/tests/integration/quadstore-utils.test.ts(1 hunks)flow-service/tests/integration/test-log-retention-config.ts(0 hunks)flow-service/tests/integration/test-sentry-init.ts(0 hunks)flow-service/tests/integration/test-sentry-logging.ts(0 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (3)
- flow-service/tests/integration/test-sentry-init.ts
- flow-service/tests/integration/test-sentry-logging.ts
- flow-service/tests/integration/test-log-retention-config.ts
🧰 Additional context used
📓 Path-based instructions (1)
.ai-guidance.md
📄 CodeRabbit Inference Engine (.clinerules/cline-guidance.md)
On every new task, first read the file
.ai-guidance.mdat the workspace root.
Files:
.ai-guidance.md
🧠 Learnings (5)
📚 Learning: applies to sflo-dendron-notes/sflo.contributors.general-guidance.md : read the file `sflo-dendron-no...
Learnt from: CR
PR: semantic-flow/sflow-platform#0
File: .clinerules/cline-guidance.md:0-0
Timestamp: 2025-07-27T01:17:36.738Z
Learning: Applies to sflo-dendron-notes/sflo.contributors.general-guidance.md : Read the file `sflo-dendron-notes/sflo.contributors.general-guidance.md` as referenced by `.ai-guidance.md`.
Applied to files:
.ai-guidance.md
📚 Learning: ensure the content of `.ai-guidance.md`, the referenced guidance files, and all ontology files is lo...
Learnt from: CR
PR: semantic-flow/sflow-platform#0
File: .clinerules/cline-guidance.md:0-0
Timestamp: 2025-07-27T01:17:36.738Z
Learning: Ensure the content of `.ai-guidance.md`, the referenced guidance files, and all ontology files is loaded into your context before you begin work.
Applied to files:
.ai-guidance.md
📚 Learning: applies to .ai-guidance.md : on every new task, first read the file `.ai-guidance.md` at the workspa...
Learnt from: CR
PR: semantic-flow/sflow-platform#0
File: .clinerules/cline-guidance.md:0-0
Timestamp: 2025-07-27T01:17:36.738Z
Learning: Applies to .ai-guidance.md : On every new task, first read the file `.ai-guidance.md` at the workspace root.
Applied to files:
.ai-guidance.md
📚 Learning: applies to sflo-dendron-notes/sflo.contributors.djradon.ai-guidance.md : read the file `sflo-dendron...
Learnt from: CR
PR: semantic-flow/sflow-platform#0
File: .clinerules/cline-guidance.md:0-0
Timestamp: 2025-07-27T01:17:36.738Z
Learning: Applies to sflo-dendron-notes/sflo.contributors.djradon.ai-guidance.md : Read the file `sflo-dendron-notes/sflo.contributors.djradon.ai-guidance.md` as referenced by `.ai-guidance.md`.
Applied to files:
.ai-guidance.md
📚 Learning: applies to ontology/** : read all ontology files under the `ontology/` folder as referenced by `.ai-...
Learnt from: CR
PR: semantic-flow/sflow-platform#0
File: .clinerules/cline-guidance.md:0-0
Timestamp: 2025-07-27T01:17:36.738Z
Learning: Applies to ontology/** : Read all ontology files under the `ontology/` folder as referenced by `.ai-guidance.md`.
Applied to files:
.ai-guidance.md
🧬 Code Graph Analysis (14)
flow-service/src/config/node-config.default.jsonld (1)
flow-service/src/config/types.ts (1)
JSONLDContext(9-15)
flow-core/src/utils/quadstore/quadstore-factory.ts (2)
flow-core/src/types.ts (1)
QuadstoreBundle(8-13)flow-core/src/deps.ts (4)
MemoryLevel(18-18)DataFactory(17-17)Quadstore(16-16)Engine(23-23)
flow-service/src/utils/logger.ts (1)
flow-service/src/config/types.ts (3)
getConsoleLogLevel(217-220)LogChannelConfig(24-36)LoggingConfig(38-43)
flow-service/src/quadstore-default-bundle.ts (2)
flow-core/src/deps.ts (4)
MemoryLevel(18-18)DataFactory(17-17)Quadstore(16-16)Engine(23-23)flow-core/src/types.ts (1)
QuadstoreBundle(8-13)
flow-service/src/config/loaders/env-loader.ts (1)
flow-service/src/config/types.ts (6)
NodeConfigInput(119-130)NodeConfig(61-72)NodeConfigContext(138-141)LogChannelConfig(24-36)ServiceConfig(75-85)LoggingConfig(38-43)
flow-service/tests/integration/logging/formatting-direct.test.ts (1)
flow-service/src/utils/logger.ts (2)
LogContext(21-67)formatConsoleMessage(221-259)
flow-core/src/platform-constants.ts (2)
flow-service/src/config/index.ts (1)
LogLevel(27-27)flow-service/src/config/types.ts (3)
LogChannelConfig(24-36)LoggingConfig(38-43)getConsoleLogLevel(217-220)
flow-core/src/types.ts (1)
flow-core/src/deps.ts (3)
Quadstore(16-16)DataFactory(17-17)Engine(23-23)
flow-service/flow-service-config.jsonld (1)
flow-service/src/config/types.ts (6)
ServiceConfig(75-85)JSONLDContext(9-15)JSONLDBase(17-21)ServiceConfigInput(107-117)NodeConfig(61-72)ContainedServicesConfig(46-53)
flow-service/tests/integration/quadstore-utils.test.ts (2)
flow-core/src/deps.ts (4)
MemoryLevel(18-18)DataFactory(17-17)Quadstore(16-16)assertEquals(3-3)flow-core/src/utils/quadstore-utils.ts (3)
clearGraph(35-50)copyGraph(54-70)countQuadsInStream(8-15)
flow-core/src/utils/rdfjs-utils.ts (1)
flow-core/src/deps.ts (3)
DataFactory(17-17)NodeObject(31-31)jsonld(21-21)
flow-service/src/config/resolution/service-config-resolver.ts (4)
flow-service/src/config/config-types.ts (2)
ServiceOptions(139-146)ServiceConfigInput(108-118)flow-service/src/config/loaders/jsonld-loader.ts (1)
loadServiceConfig(22-74)flow-service/src/utils/merge-configs.ts (1)
mergeConfigs(16-39)flow-service/src/config/types.ts (3)
ServiceConfigContext(133-136)ServiceConfigInput(107-117)ServiceConfig(75-85)
flow-core/src/utils/quadstore-utils.ts (3)
flow-service/src/quadstore-default-bundle.ts (1)
defaultQuadstoreBundle(11-11)flow-core/src/utils/rdfjs-utils.ts (1)
jsonldToQuads(9-25)flow-core/src/types.ts (1)
QuadstoreBundle(8-13)
flow-service/src/utils/mesh-utils.ts (1)
flow-service/src/config/types.ts (1)
ServiceConfig(75-85)
🔇 Additional comments (45)
flow-service/deno.json (2)
11-14: Verify relative log path & consistency of--env-fileflag
--allow-write=../meshes,./logsassumesflow-service/logsexists. Confirm tasks create or vendor-ignore the directory to avoid runtime errors.- In
"dev"the flag--env-fileis present without a path. Deno treats this as “use.env”, but"start"passesmain.tsas the path. Confirm this asymmetry is intentional.No blocking issues, just double-check behaviour.
21-21: Formatter setting aligns with new ESLint quote style – looks gooddeno.json (1)
18-19: Global quote preference update acknowledged
Consistent withflow-service/deno.jsonand ESLint rules.flow-core/src/platform-constants.ts (2)
1-3: LGTM! Excellent use of const assertion for type safety.The combination of
as constassertion withtypeof validLogLevels[number]creates a proper union type from the array values, ensuring type safety at compile time.
12-19: Well-implemented validation with comprehensive error handling.The validation function properly checks both type and value constraints, provides clear error messages, and uses appropriate type assertions. The implementation follows TypeScript best practices for runtime validation.
flow-core/src/deps.ts (2)
20-21: Correct handling of JSON-LD default export.The import and re-export pattern properly handles the default export from the jsonld npm package, making it available as a named export for consistent usage patterns.
16-24: Dependency versions and security verifiedAll newly added packages are at their latest released versions and have no reported security advisories:
- quadstore @ 15.3.0
- rdf-data-factory @ 2.0.2
- memory-level @ 3.1.0
- quadstore-comunica @ 6.2.0
- jsonld @ 8.3.3
- @types/jsonld @ 1.5.15
- @rdfjs/types @ 2.0.1
No vulnerabilities were found via the NPM registry or GitHub advisories.
flow-service/src/config/node-config.default.jsonld (1)
9-9: Consistent MeshRootNodeConfig identifier usage verified
- No remaining references to
defaultNodeConfigfound.- Only occurrence of
defaultMeshRootNodeConfigis in
flow-service/src/config/node-config.default.jsonldat line 9.No further updates required.
flow-service/tests/integration/logging/pretty-format-simple.test.ts (1)
11-11: Import path is correctThe import in
flow-service/tests/integration/logging/pretty-format-simple.test.tsimport { logger } from '../../../src/utils/logger.ts';correctly resolves to
flow-service/src/utils/logger.ts. No further changes are needed.flow-service/tests/integration/logging/logger.test.ts (1)
7-7: Import path update is consistent with directory structure changes.This change aligns with the similar update in
pretty-format-simple.test.ts. The pattern suggests the test files were reorganized or the logger module was relocated.flow-service/src/config/loaders/env-loader.ts (2)
227-227: MeshRootNodeConfig type change verified
The@typeupdate to'conf:MeshRootNodeConfig'matches the interface inconfig-types.tsand aligns with all usages indefaults.ts,index.ts, and loader modules. No remaining references to'conf:NodeConfig'were found.
11-13: External validateLogLevel Import and Behavior VerifiedThe
validateLogLevelfunction inflow-core/src/platform-constants.ts:
- Has the signature
validateLogLevel(level: unknown): LogLevel- Throws a descriptive
Errorwhen the input is not a valid log level- Returns the validated
LogLevelotherwiseIn
env-loader.ts, any thrownErroris caught and wrapped (or re-thrown if already aConfigError), ensuring invalid log levels still surface asConfigError—matching the original local implementation’s semantics.No changes required.
flow-service/src/routes/weave.ts (2)
19-20: Configuration parameter removal aligns with singleton pattern.The removal of the
configparameter fromcreateWeaveRoutesis consistent with the architectural shift to singleton configuration management. Ensure that any configuration access needed within route handlers uses the appropriate singleton accessor.
93-95: Minor formatting improvement.The logging statement formatting change improves readability without affecting functionality.
flow-core/src/utils/quadstore/quadstore-factory.ts (1)
1-17: Well-structured factory function.The factory function provides good encapsulation of quadstore bundle creation with proper async handling and clear return types.
flow-core/src/types.ts (1)
8-13: Well-designed interface with good type safety.The
QuadstoreBundleinterface provides excellent type safety with appropriate generic constraints and flexibility with the optionalengineproperty. The design supports various backend implementations while maintaining type consistency.flow-service/tests/integration/quadstore-accessor.test.ts (1)
5-44: Well-structured integration test with comprehensive coverage.The test effectively validates the complete Quadstore workflow from data insertion to SPARQL querying. The test data structure, query construction, and assertions are all appropriate for verifying the integration functionality.
flow-core/src/utils/rdfjs-utils.ts (1)
9-25: Excellent implementation of JSON-LD to RDF conversion.The function correctly uses
jsonld.toRDF()for conversion and properly handles graph assignment. The logic for distinguishing between default and named graphs is sound, and the quad remapping preserves all RDF components while updating the graph context appropriately.flow-service/src/utils/mesh-utils.ts (1)
3-3: Good refactoring to use singleton configuration accessor.The import change from a parameterized config to the singleton accessor aligns well with the architectural refactor described in the PR objectives.
flow-service/src/services/metadata-composer.ts (1)
1-1: Good refactoring to singleton configuration accessor.The import change aligns with the architectural shift to using a singleton configuration accessor, eliminating the need to pass config as a parameter.
eslint.config.mjs (1)
4-19: Well-configured ESLint setup for TypeScript.The configuration properly targets TypeScript files, uses the appropriate parser, and enforces consistent string quoting that aligns with the codebase formatting changes. The flat config format is correctly structured.
flow-service/main.ts (2)
15-15: Good error handling for configuration initializationThe error handling properly catches configuration failures and exits the service, preventing it from running with invalid configuration.
5-5: No initialization race detectedThe
defaultQuadstoreBundleuses a top-level await to open the Quadstore before exporting the engine, and in main.ts we call and awaitcreateServiceConfig()(which loads and merges all graphs) before anysingletonServiceConfigAccessor.get…()calls. The accessor is fully initialized and the store is opened prior to its first use, so no race condition exists.flow-service/flow-service-config.jsonld (1)
15-16: Configuration values match validation requirementsThe addition of
fsvc:hostensures the configuration passes validation, and the port value is within the valid range (1-65535).flow-service/tests/integration/logging/formatting-direct.test.ts (1)
47-48: Good refactoring to use shared formatting functionUsing the exported
formatConsoleMessagefrom the logger module instead of a local implementation ensures the test validates the actual production formatting logic.Also applies to: 125-125
flow-core/tests/unit/quadstore-utils.test.ts (2)
5-24: Good test structure and isolationThe tests properly isolate each scenario with unique graph URNs and clean up before and after operations. The assertions are clear and test the expected behavior.
64-85: Add cleanup for graphs in copyGraph testThe test should clean up both source and target graphs after the test completes for consistency with other tests.
assertEquals(countSource, 1); assertEquals(countTarget, 1); + + // Cleanup + await clearGraph(sourceGraph, testQuadstoreBundle); + await clearGraph(targetGraph, testQuadstoreBundle); });Likely an incorrect or invalid review comment.
flow-service/src/config/resolution/service-config-loader.ts (1)
28-33: No changes needed for error handling in loadServiceConfigThe JSDoc on
loadServiceConfig(jsonld-loader.ts) clearly specifies that it:
- Returns
nullwhen the file is missing- Throws a
ConfigErrorfor invalid JSON, wrong@type, or other failuresSince callers of
loadServiceConfigdon’t catch errors, any parsing or validation failures will bubble up as intended. No additional error‐handling or documentation updates are required here.flow-service/src/config/resolution/service-config-accessor.ts (1)
70-72: Remove duplicate SPARQL engine checkThe engine check is redundant since
getMeshPathsdoesn't usequerySingleValuebut should follow the same pattern.Consider refactoring
getMeshPathsto use a similar pattern or extract the engine check to a shared method:async getMeshPaths(): Promise<string[]> { const sparql = `...`; - if (!this.bundle.engine) { - throw new Error('SPARQL engine not initialized in Quadstore bundle'); - } const values: string[] = []; - const bindingsStream = await this.bundle.engine.queryBindings(sparql, { sources: [this.bundle.store] }); + try { + if (!this.bundle.engine) { + throw new Error('SPARQL engine not initialized in Quadstore bundle'); + } + const bindingsStream = await this.bundle.engine.queryBindings(sparql, { sources: [this.bundle.store] }); + } catch (error) { + await handleCaughtError(error, 'Failed to execute SPARQL query for mesh paths'); + throw new Error('Failed to execute SPARQL query for mesh paths'); + }Likely an incorrect or invalid review comment.
flow-service/src/config/loaders/jsonld-loader.ts (7)
8-12: LGTM! Clean import restructuring.The imports have been properly updated to use the new
MeshRootNodeConfigInputtype and reorganized config-types imports.
22-74: Well-structured error handling and validation.The function properly handles all error cases with appropriate error messages and maintains the error chain through the
causeparameter.
84-140: Consistent renaming and proper path resolution.The function has been correctly renamed to match the new
MeshRootNodeConfignaming convention and includes proper absolute path resolution to prevent relative path issues.
147-171: LGTM! Standard save implementation.The function correctly handles directory creation and JSON formatting with proper error handling.
178-204: Consistent renaming with proper implementation.The function has been correctly renamed and updated to use the new
MeshRootNodeConfigInputtype while maintaining the same robust implementation pattern.
250-267: Proper handling of optional configuration.The function correctly defaults to
truefor inheritance when the property is unspecified or when configuration cannot be loaded, which is a sensible default behavior.
274-282: Improved function naming and type safety.The function has been renamed to follow camelCase convention and the parameter type has been appropriately narrowed to
NodeObject, eliminating the need for the initial object check.flow-service/src/config/loaders/quadstore-loader.ts (1)
1-5: Good modular design with clear separation of concerns.The imports are well-organized, separating quadstore utilities from configuration types and constants.
flow-core/src/utils/quadstore-utils.ts (1)
8-15: Well-implemented stream counting utility.The function correctly handles RDF streams with proper event handling for data, end, and error events.
flow-service/src/config/resolution/service-config-resolver.ts (2)
13-22: Clean import organization for the new graph-based approach.The imports clearly separate concerns between platform constants, loaders, and quadstore operations.
24-71: Well-structured graph-based configuration resolution.The refactored function follows a clear flow: load platform defaults → load and merge input configs → load to Quadstore → merge graphs. The error handling is comprehensive and maintains proper error context.
flow-service/src/config/config-types.ts (1)
8-9: Good use of shared JSON-LD types.Importing
NodeObjectandContextDefinitionfrom core deps promotes type consistency across the codebase.flow-service/src/config/defaults.ts (2)
8-206: Well-structured configuration refactoringThe configuration defaults have been successfully refactored to support the new graph-based approach:
- Consistent use of JSON-LD structure with explicit
@idfields- Proper typing with the new
MeshRootNodeConfigtype- Clean separation of platform vs service defaults
- Environment-specific overrides are well-organized
The changes align well with the PR's objective of implementing a singleton pattern for configuration data management.
37-37: Verify the updated template pathsI couldn’t locate
default-resource-page.htmlunder/_assets/_templates/. Please confirm that this path aligns with your project’s static‐assets directory. If the actual folder structure differs, update the template references inflow-service/src/config/defaults.tsaccordingly.Points to check:
- No file found at
/_assets/_templates/default-resource-page.html- Does the file live under a different path? (e.g.,
/assets/_templates/,/assets/templates/, etc.)- Update lines 37 (and 68) in
flow-service/src/config/defaults.tsto match the real directory layout.flow-service/src/config/index.ts (2)
89-99: Clean separation of concerns in createServiceConfigThe simplified
createServiceConfigfunction now properly follows the single responsibility principle by only orchestrating resolution and validation, without returning an accessor. This aligns well with the singleton pattern where the accessor is accessed separately viasingletonServiceConfigAccessor.
1-100: Excellent modularization of configuration systemThe refactoring successfully modularizes the configuration system:
- Clear separation between resolution, validation, and access concerns
- Proper use of singleton pattern for configuration access
- Well-organized exports that expose only necessary interfaces
- Clean transition from in-memory merging to graph-based configuration
The changes create a more maintainable and scalable configuration management system.
| export async function clearGraph( | ||
| graph: RDF.NamedNode, | ||
| { | ||
| store | ||
| }: { store: Quadstore } = defaultQuadstoreBundle | ||
| ): Promise<number> { | ||
| const matchStream = store.match(undefined, undefined, undefined, graph); | ||
| const count = await countQuadsInStream(matchStream); | ||
|
|
||
| if (count > 0) { | ||
| const delStream = store.match(undefined, undefined, undefined, graph); | ||
| await store.delStream(delStream as any); | ||
| } | ||
|
|
||
| return count; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize graph clearing and fix type assertion.
The function performs two passes over the data when one would suffice. Also, the as any type assertion should be avoided.
export async function clearGraph(
graph: RDF.NamedNode,
{
store
}: { store: Quadstore } = defaultQuadstoreBundle
): Promise<number> {
- const matchStream = store.match(undefined, undefined, undefined, graph);
- const count = await countQuadsInStream(matchStream);
-
- if (count > 0) {
- const delStream = store.match(undefined, undefined, undefined, graph);
- await store.delStream(delStream as any);
- }
-
- return count;
+ const quads = [];
+ const stream = store.match(undefined, undefined, undefined, graph);
+
+ for await (const quad of stream) {
+ quads.push(quad);
+ }
+
+ if (quads.length > 0) {
+ await store.multiDel(quads);
+ }
+
+ return quads.length;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In flow-core/src/utils/quadstore-utils.ts lines 35 to 50, the clearGraph
function currently matches the graph twice and uses an unsafe 'as any' type
assertion on delStream. Refactor the function to perform a single match stream
and delete operation without counting first, and replace the 'as any' assertion
with the correct type expected by store.delStream to ensure type safety.
| export async function copyGraph( | ||
| sourceGraph: RDF.NamedNode, | ||
| targetGraph: RDF.NamedNode, | ||
| { | ||
| store, | ||
| df, | ||
| }: { store: Quadstore, df: DataFactory } = defaultQuadstoreBundle | ||
| ): Promise<number> { | ||
| const stream = store.match(undefined, undefined, undefined, sourceGraph); | ||
| const quads = []; | ||
| for await (const q of stream as any) { | ||
| const newQuad = df.quad(q.subject, q.predicate, q.object, targetGraph); | ||
| quads.push(newQuad); | ||
| } | ||
| await store.multiPut(quads); | ||
| return quads.length; | ||
| } |
There was a problem hiding this comment.
Fix type assertion and consider memory usage for large graphs.
The function loads all quads into memory before copying, which could cause issues with large graphs. Also, avoid the as any type assertion.
export async function copyGraph(
sourceGraph: RDF.NamedNode,
targetGraph: RDF.NamedNode,
{
store,
df,
}: { store: Quadstore, df: DataFactory } = defaultQuadstoreBundle
): Promise<number> {
const stream = store.match(undefined, undefined, undefined, sourceGraph);
const quads = [];
- for await (const q of stream as any) {
+ for await (const q of stream) {
const newQuad = df.quad(q.subject, q.predicate, q.object, targetGraph);
quads.push(newQuad);
}
await store.multiPut(quads);
return quads.length;
}For very large graphs, consider implementing a streaming copy that processes quads in batches to avoid memory issues.
📝 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.
| export async function copyGraph( | |
| sourceGraph: RDF.NamedNode, | |
| targetGraph: RDF.NamedNode, | |
| { | |
| store, | |
| df, | |
| }: { store: Quadstore, df: DataFactory } = defaultQuadstoreBundle | |
| ): Promise<number> { | |
| const stream = store.match(undefined, undefined, undefined, sourceGraph); | |
| const quads = []; | |
| for await (const q of stream as any) { | |
| const newQuad = df.quad(q.subject, q.predicate, q.object, targetGraph); | |
| quads.push(newQuad); | |
| } | |
| await store.multiPut(quads); | |
| return quads.length; | |
| } | |
| export async function copyGraph( | |
| sourceGraph: RDF.NamedNode, | |
| targetGraph: RDF.NamedNode, | |
| { | |
| store, | |
| df, | |
| }: { store: Quadstore, df: DataFactory } = defaultQuadstoreBundle | |
| ): Promise<number> { | |
| const stream = store.match(undefined, undefined, undefined, sourceGraph); | |
| const quads = []; | |
| for await (const q of stream) { | |
| const newQuad = df.quad(q.subject, q.predicate, q.object, targetGraph); | |
| quads.push(newQuad); | |
| } | |
| await store.multiPut(quads); | |
| return quads.length; | |
| } |
🤖 Prompt for AI Agents
In flow-core/src/utils/quadstore-utils.ts around lines 54 to 70, the function
currently collects all quads into an array before writing, which can cause high
memory usage for large graphs, and it uses an unsafe 'as any' type assertion. To
fix this, remove the 'as any' assertion by properly typing the stream, and
refactor the function to process quads in batches by accumulating a limited
number of quads before calling store.multiPut, repeating until all quads are
processed, to reduce memory consumption.
| export async function createNewGraphFromJsonLd( | ||
| inputJsonLd: NodeObject, | ||
| options: CreateGraphOptions = {} | ||
| ): Promise<number> { | ||
| const { | ||
| overwrite = false, | ||
| graphName = null, | ||
| bundle = defaultQuadstoreBundle, | ||
| } = options; | ||
|
|
||
| const { store, df } = bundle; | ||
| const graph = graphName ? df.namedNode(graphName) : df.defaultGraph(); | ||
| const quads = await jsonldToQuads(inputJsonLd, graph); | ||
| if (overwrite && graphName) clearGraph(df.namedNode(graphName)); | ||
|
|
||
| // Put quads into the store | ||
| await store.multiPut(quads); | ||
|
|
||
| return quads.length; | ||
| } |
There was a problem hiding this comment.
Critical: Missing await for async clearGraph call.
The clearGraph function is asynchronous but is not awaited on line 123, which could lead to race conditions where quads are inserted before the graph is cleared.
const { store, df } = bundle;
const graph = graphName ? df.namedNode(graphName) : df.defaultGraph();
const quads = await jsonldToQuads(inputJsonLd, graph);
- if (overwrite && graphName) clearGraph(df.namedNode(graphName));
+ if (overwrite && graphName) await clearGraph(df.namedNode(graphName), bundle);
// Put quads into the store
await store.multiPut(quads);
return quads.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.
| export async function createNewGraphFromJsonLd( | |
| inputJsonLd: NodeObject, | |
| options: CreateGraphOptions = {} | |
| ): Promise<number> { | |
| const { | |
| overwrite = false, | |
| graphName = null, | |
| bundle = defaultQuadstoreBundle, | |
| } = options; | |
| const { store, df } = bundle; | |
| const graph = graphName ? df.namedNode(graphName) : df.defaultGraph(); | |
| const quads = await jsonldToQuads(inputJsonLd, graph); | |
| if (overwrite && graphName) clearGraph(df.namedNode(graphName)); | |
| // Put quads into the store | |
| await store.multiPut(quads); | |
| return quads.length; | |
| } | |
| export async function createNewGraphFromJsonLd( | |
| inputJsonLd: NodeObject, | |
| options: CreateGraphOptions = {} | |
| ): Promise<number> { | |
| const { | |
| overwrite = false, | |
| graphName = null, | |
| bundle = defaultQuadstoreBundle, | |
| } = options; | |
| const { store, df } = bundle; | |
| const graph = graphName ? df.namedNode(graphName) : df.defaultGraph(); | |
| const quads = await jsonldToQuads(inputJsonLd, graph); | |
| if (overwrite && graphName) await clearGraph(df.namedNode(graphName), bundle); | |
| // Put quads into the store | |
| await store.multiPut(quads); | |
| return quads.length; | |
| } |
🤖 Prompt for AI Agents
In flow-core/src/utils/quadstore-utils.ts lines 110 to 129, the call to the
asynchronous function clearGraph on line 123 is missing an await, which can
cause race conditions by inserting quads before the graph is cleared. To fix
this, add the await keyword before clearGraph to ensure the graph is fully
cleared before proceeding with inserting new quads.
| const meshPaths = await config.getMeshPaths(); | ||
| if (meshPaths.length > 0) { | ||
| for (const meshPath of meshPaths) { | ||
| const absolutePath = resolve(Deno.cwd(), meshPath); | ||
| console.log(` Configured mesh path: ${absolutePath}`); | ||
| } | ||
| } else { | ||
| console.log(` Mesh Paths: none configured`); | ||
| } |
There was a problem hiding this comment.
Add error handling for async config operations
The async config accessor calls could fail but there's no error handling.
Wrap the config calls in try-catch blocks:
export async function logStartupConfiguration(): Promise<void> {
const now = new Date();
// ... timestamp code ...
console.log(
`🔧 Flow Service initializing at ${timestamp} with configuration:`,
);
- const meshPaths = await config.getMeshPaths();
+ try {
+ const meshPaths = await config.getMeshPaths();
+ } catch (error) {
+ console.error('Failed to retrieve mesh paths:', error);
+ const meshPaths = [];
+ }Apply similar error handling to logStartupUrls().
Also applies to: 63-65
🤖 Prompt for AI Agents
In flow-service/src/utils/startup-logger.ts around lines 32 to 40 and also lines
63 to 65, the async calls to config.getMeshPaths() and other config accessors
lack error handling. Wrap these async calls in try-catch blocks to catch and log
any errors that occur during the config retrieval. Apply the same try-catch
error handling pattern to the logStartupUrls() function to ensure all async
config operations are safely handled.
|
|
||
| console.log(`📍 Root: ${baseUrl}/`); | ||
| console.log(`📚 API documentation: ${baseUrl}${MESH.API_PORTAL_ROUTE}`); | ||
| console.log(`� API documentation: ${baseUrl}${MESH.API_PORTAL_ROUTE}`); |
There was a problem hiding this comment.
Fix broken Unicode character
The Unicode character appears corrupted in the log output.
- console.log(`� API documentation: ${baseUrl}${MESH.API_PORTAL_ROUTE}`);
+ console.log(`📖 API documentation: ${baseUrl}${MESH.API_PORTAL_ROUTE}`);📝 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.
| console.log(`� API documentation: ${baseUrl}${MESH.API_PORTAL_ROUTE}`); | |
| console.log(`📖 API documentation: ${baseUrl}${MESH.API_PORTAL_ROUTE}`); |
🤖 Prompt for AI Agents
In flow-service/src/utils/startup-logger.ts at line 68, the Unicode character in
the console.log statement is corrupted and not displaying correctly. Replace the
broken Unicode character with a valid and intended Unicode symbol or remove it
entirely to ensure proper display in the log output.
| Deno.test('Quadstore clearGraph and copyGraph utilities', async () => { | ||
| const backend = new MemoryLevel(); | ||
| const df = new DataFactory(); | ||
| const store = new Quadstore({ backend, dataFactory: df }); | ||
| await store.open(); |
There was a problem hiding this comment.
Add proper cleanup by closing the store after test completion
The test opens the quadstore but never closes it, which could lead to resource leaks in the test suite.
Add cleanup at the end of the test:
// Verify graphB still has 1 quad
count = await countQuadsInGraph(graphB, store);
assertEquals(count, 1);
+
+ await store.close();
});📝 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.
| Deno.test('Quadstore clearGraph and copyGraph utilities', async () => { | |
| const backend = new MemoryLevel(); | |
| const df = new DataFactory(); | |
| const store = new Quadstore({ backend, dataFactory: df }); | |
| await store.open(); | |
| Deno.test('Quadstore clearGraph and copyGraph utilities', async () => { | |
| const backend = new MemoryLevel(); | |
| const df = new DataFactory(); | |
| const store = new Quadstore({ backend, dataFactory: df }); | |
| await store.open(); | |
| // … the rest of your test setup and assertions … | |
| // Verify graphB still has 1 quad | |
| let count = await countQuadsInGraph(graphB, store); | |
| assertEquals(count, 1); | |
| await store.close(); | |
| }); |
🤖 Prompt for AI Agents
In flow-service/tests/integration/quadstore-utils.test.ts around lines 6 to 10,
the test opens the quadstore but does not close it, risking resource leaks. Add
a call to store.close() at the end of the test to properly clean up resources
after test completion.
- Updated the import of Quadstore in deps.ts to use the beta version. - Modified clearGraph function to remove return count and log the number of quads after deletion. - Enhanced error handling in createQuadstoreBundle to catch and log errors when opening the quadstore. - Updated tests for clearGraph to reflect changes in return type and added assertions for graph state before and after clearing. - Cleaned up type assertions in various files to improve type safety. - Removed unnecessary console logs and comments in service configuration loaders and validators.
- Updated metadata-composer.ts to use double quotes for consistency and modified the attributedTo field to use a hardcoded URL. - Refactored error handling in errors.ts to improve structured logging and context management. - Removed the logger.ts file and replaced it with a new service-logger.ts that provides service-specific logging configuration and context. - Updated mesh-utils.ts to use the new service logger and improved logging context for mesh registration. - Adjusted startup-logger.ts to ensure type safety for enabled services. - Modified various test files to import from the new service-logger.ts and updated logging calls to align with the new structured logging format.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
flow-service/main.ts (1)
110-111: Route creation functions still need singleton accessor integration.As noted in the previous review,
createMeshesRoutes()andcreateWeaveRoutes()no longer receive a config parameter but haven't been updated to usesingletonServiceConfigAccessor()internally. The implementations inflow-service/src/routes/meshes.tsandflow-service/src/routes/weave.tsneed to import and call the singleton accessor for any configuration they require.
🧹 Nitpick comments (9)
flow-core/tests/integration/logger/formatters.test.ts (1)
1-147: Consider using a test framework for automated verification.While these integration tests effectively demonstrate the formatting functions, they rely on manual verification of console output. Consider using Deno's built-in test framework with assertions for automated testing:
Example refactor for one test:
import { assertEquals } from "https://deno.land/std/testing/asserts.ts"; Deno.test("formatConsoleMessage should format messages correctly", () => { const context: LogContext = { operation: 'startup', component: 'config-resolver', serviceContext: { serviceName: 'flow-core-test', serviceVersion: '1.0.0' } }; const result = formatConsoleMessage('INFO', 'Test message', context); // Assert the result contains expected parts assert(result.includes('INFO')); assert(result.includes('Test message')); assert(result.includes('startup')); });flow-core/src/utils/logger/structured-logger.ts (1)
194-215: Consider optimizing file rotation checks.The
rotateIfNeeded()is called on every write operation, which could impact performance for high-frequency logging. Consider implementing a time-based or write-count-based check to reduce filesystem operations.private async writeToFile( level: LogLevel, message: string, context?: LogContext, ): Promise<void> { try { const fileLogger = await getGlobalFileLogger(); if (fileLogger) { - await fileLogger.rotateIfNeeded(); + // Only check rotation periodically or based on write count + // This could be managed by the FileLogger internally const fileFormatted = formatStructuredMessage( level, message, context, this.config.serviceContext, ); await fileLogger.writeToFile(fileFormatted); } } catch (err) { console.error(`Failed to write to file log: ${err}`); } }flow-core/src/utils/logger/file-logger.ts (1)
43-56: Consider making queue overflow handling configurable.When the queue is full, the logger drops the oldest half of entries and logs to console. Consider making this behavior configurable (e.g., drop oldest, drop newest, or block) and potentially emit an event for monitoring.
async writeToFile(content: string): Promise<void> { if (this.writeQueue.length >= this.maxQueueSize) { console.error( `Log queue full (${this.maxQueueSize} items), dropping oldest entries`, ); + // Consider emitting an event or metric for monitoring + // this.emit('queueOverflow', { queueSize: this.writeQueue.length }); this.writeQueue = this.writeQueue.slice( -Math.floor(this.maxQueueSize / 2), ); } this.writeQueue.push(content); if (!this.isWriting) { await this.processWriteQueue(); } }flow-service/src/config/resolution/service-config-resolver.ts (1)
54-56: Track the TODO for mesh node configuration loading.The TODO comment indicates missing functionality for loading mesh node configuration. This should be tracked properly to ensure it's not forgotten.
Would you like me to create an issue to track the implementation of
loadInputMeshRootNodeConfig?flow-core/src/utils/logger/error-handlers.ts (1)
88-400: Consider refactoring for improved maintainability.While the comprehensive error handling is excellent, the function is quite long (300+ lines). Consider extracting error-type-specific handlers into separate functions to improve readability and maintainability.
// Example refactor approach: private async logFlowServiceError( error: Error & { code?: string; context?: unknown }, formatErrorMsg: (type: string, msg: string) => string, baseContext: LogContext ): Promise<void> { // Extract FlowServiceError handling logic } private async logValidationError( error: Error & { code?: string; field?: string; context?: unknown }, formatErrorMsg: (type: string, msg: string) => string, baseContext: LogContext ): Promise<void> { // Extract ValidationError handling logic } // Then in handleCaughtError: if (e.name === 'FlowServiceError') { await this.logFlowServiceError(e, formatErrorMsg, baseContext); } else if (e.name === 'ValidationError') { await this.logValidationError(e, formatErrorMsg, baseContext); } // etc.flow-service/src/config/config-types.ts (1)
60-61: Complete the TODO commentThe TODO comment on line 60 appears to be incomplete. Please provide the intended new name for the interface to maintain code clarity.
-// TODO rename to +// TODO rename to <intended_name>flow-core/src/utils/logger/sentry-logger.ts (1)
14-15: Module-level mutable state may cause issuesThe
sentryInitializedvariable is module-level mutable state that could cause issues in concurrent or testing scenarios. TheresetSentryfunction allows external modification of this state, which could lead to unexpected behavior.Consider encapsulating the initialization state in a class or using a more robust singleton pattern to manage Sentry's lifecycle, especially since this is meant to be used across the application.
Also applies to: 276-278
flow-core/src/utils/logger/formatters.ts (1)
201-255: Consider edge cases in context mergingThe
mergeLogContextfunction performs shallow merging for nested objects. This approach might lose deeply nested properties if they exist at multiple levels.Consider using a more robust deep merge utility or documenting the shallow merge behavior to set clear expectations:
+/** + * Merge multiple LogContext objects with proper precedence + * @param contexts - Array of LogContext objects to merge (later entries take precedence) + * @returns Merged LogContext + * @note Performs shallow merge on nested objects (performanceMetrics, configContext, etc.) + */ export function mergeLogContext(...contexts: (LogContext | undefined)[]): LogContext {flow-service/src/config/index.ts (1)
11-18: Consider making graph URIs configurableThe graph URIs are hardcoded to use
http://localhost/graph/. This might not be suitable for all deployment environments or when multiple instances are running.Consider making these URIs configurable through environment variables or configuration:
+const GRAPH_BASE_URI = Deno.env.get('FLOW_GRAPH_BASE_URI') || 'http://localhost/graph/'; + export const CONFIG_GRAPH_NAMES = { - platformServiceDefaults: 'http://localhost/graph/platformServiceDefaults', - platformImplicitMeshRootNodeConfig: 'http://localhost/graph/platformImplicitMeshRootNodeConfig', - inputServiceConfig: 'http://localhost/graph/inputServiceConfig', - inputMeshRootNodeConfig: 'http://localhost/graph/inputMeshRootNodeConfig', - mergedServiceConfig: 'http://localhost/graph/mergedServiceConfig', + platformServiceDefaults: `${GRAPH_BASE_URI}platformServiceDefaults`, + platformImplicitMeshRootNodeConfig: `${GRAPH_BASE_URI}platformImplicitMeshRootNodeConfig`, + inputServiceConfig: `${GRAPH_BASE_URI}inputServiceConfig`, + inputMeshRootNodeConfig: `${GRAPH_BASE_URI}inputMeshRootNodeConfig`, + mergedServiceConfig: `${GRAPH_BASE_URI}mergedServiceConfig`, // Per-node effective configs could be stored with dynamic graph names };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
deno.lockis excluded by!**/*.lockflow-service/deno.lockis excluded by!**/*.lockflow-service/logs/test-pretty-direct.logis excluded by!**/*.log
📒 Files selected for processing (38)
.ai-guidance.md(1 hunks)flow-core/docs/error-handling-usage.md(1 hunks)flow-core/src/deps.ts(1 hunks)flow-core/src/utils/logger/error-handlers.ts(1 hunks)flow-core/src/utils/logger/file-logger.ts(1 hunks)flow-core/src/utils/logger/formatters.ts(1 hunks)flow-core/src/utils/logger/index.ts(1 hunks)flow-core/src/utils/logger/sentry-logger.ts(1 hunks)flow-core/src/utils/logger/structured-logger.ts(1 hunks)flow-core/src/utils/logger/types.ts(1 hunks)flow-core/src/utils/quadstore-utils.ts(1 hunks)flow-core/src/utils/quadstore/quadstore-factory.ts(1 hunks)flow-core/tests/integration/logger/error-handlers.test.ts(1 hunks)flow-core/tests/integration/logger/formatters.test.ts(1 hunks)flow-core/tests/integration/logger/logger.test.ts(1 hunks)flow-service/deno.json(1 hunks)flow-service/main.ts(5 hunks)flow-service/src/config/config-types.ts(6 hunks)flow-service/src/config/index.ts(2 hunks)flow-service/src/config/loaders/jsonld-loader.ts(7 hunks)flow-service/src/config/node-config.default.jsonld(2 hunks)flow-service/src/config/resolution/service-config-accessor.ts(1 hunks)flow-service/src/config/resolution/service-config-resolver.ts(1 hunks)flow-service/src/routes/health.ts(3 hunks)flow-service/src/routes/meshes.ts(5 hunks)flow-service/src/routes/weave.ts(3 hunks)flow-service/src/services/metadata-composer.ts(2 hunks)flow-service/src/utils/errors.ts(2 hunks)flow-service/src/utils/logger.ts(0 hunks)flow-service/src/utils/mesh-utils.ts(2 hunks)flow-service/src/utils/service-logger.ts(1 hunks)flow-service/src/utils/startup-logger.ts(3 hunks)flow-service/tests/integration/logging/formatting-direct.test.ts(3 hunks)flow-service/tests/integration/logging/handle-caught-error.test.ts(1 hunks)flow-service/tests/integration/logging/logger.test.ts(1 hunks)flow-service/tests/integration/logging/pretty-format-simple.test.ts(2 hunks)flow-service/tests/integration/logging/sentry-init.test.ts(1 hunks)flow-service/tests/integration/logging/sentry-logging.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- flow-service/src/utils/logger.ts
✅ Files skipped from review due to trivial changes (2)
- flow-service/src/config/node-config.default.jsonld
- flow-service/tests/integration/logging/handle-caught-error.test.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- flow-service/tests/integration/logging/sentry-init.test.ts
- .ai-guidance.md
- flow-service/deno.json
- flow-service/tests/integration/logging/sentry-logging.test.ts
- flow-service/tests/integration/logging/pretty-format-simple.test.ts
- flow-service/src/utils/errors.ts
- flow-service/src/routes/weave.ts
- flow-service/tests/integration/logging/logger.test.ts
- flow-core/src/utils/quadstore/quadstore-factory.ts
- flow-service/src/utils/startup-logger.ts
- flow-service/tests/integration/logging/formatting-direct.test.ts
- flow-service/src/services/metadata-composer.ts
- flow-service/src/config/resolution/service-config-accessor.ts
- flow-service/src/routes/meshes.ts
- flow-service/src/config/loaders/jsonld-loader.ts
- flow-core/src/utils/quadstore-utils.ts
🧰 Additional context used
🧬 Code Graph Analysis (5)
flow-service/src/routes/health.ts (1)
flow-service/src/utils/service-logger.ts (1)
logger(57-57)
flow-core/tests/integration/logger/formatters.test.ts (3)
flow-core/src/utils/logger/index.ts (5)
LogContext(8-8)formatConsoleMessage(24-24)formatStructuredMessage(23-23)createContextSummary(26-26)mergeLogContext(27-27)flow-core/src/utils/logger/types.ts (1)
LogContext(11-78)flow-core/src/utils/logger/formatters.ts (4)
formatConsoleMessage(112-152)formatStructuredMessage(79-103)createContextSummary(182-194)mergeLogContext(201-255)
flow-core/src/utils/logger/types.ts (4)
flow-core/src/utils/logger/index.ts (7)
LogContext(8-8)StructuredLogger(9-9)LogLevel(15-15)LogLevel(18-18)LoggerConfig(10-10)ErrorSeverity(11-11)ErrorHandlingOptions(12-12)flow-service/src/config/index.ts (1)
LogLevel(27-27)flow-service/src/config/config-types.ts (1)
LogLevel(186-186)flow-service/src/config/types.ts (1)
LogChannelConfig(24-36)
flow-core/src/utils/logger/index.ts (2)
flow-core/src/utils/logger/types.ts (1)
StructuredLogger(84-146)flow-core/src/utils/logger/structured-logger.ts (3)
createLogger(331-336)EnhancedStructuredLogger(277-323)createEnhancedLogger(344-349)
flow-service/src/utils/service-logger.ts (3)
flow-core/src/utils/logger/index.ts (5)
LoggerConfig(10-10)EnhancedStructuredLogger(33-33)createEnhancedLogger(35-35)StructuredLogger(9-9)LogContext(8-8)flow-core/src/utils/logger/types.ts (3)
LoggerConfig(163-199)StructuredLogger(84-146)LogContext(11-78)flow-service/src/config/loaders/env-loader.ts (1)
loadEnvConfig(18-209)
🔇 Additional comments (27)
flow-service/src/routes/health.ts (1)
2-2: LGTM! Structured logging improvements enhance observability.The migration from the legacy logger to the new structured logger with rich contextual metadata is well-implemented. The new format provides better observability with operation categorization and proper nesting of API context information.
Also applies to: 38-46, 58-61
flow-core/tests/integration/logger/logger.test.ts (1)
1-94: Excellent comprehensive integration test for the logging system.This test effectively validates the enhanced structured logger functionality with:
- Multiple log levels with rich contextual metadata
- Performance metrics, API context, error context, and configuration context
- Contextual logger feature (
withContext) validation- Clear user guidance for verification
The test coverage is thorough and follows good practices for integration testing.
flow-service/main.ts (1)
16-31: LGTM! Configuration refactoring and error handling improvements are well-implemented.The migration to the singleton service config accessor pattern with proper async handling and the enhanced structured error handling with rich LogContext metadata significantly improve the codebase maintainability and observability.
Also applies to: 65-68, 134-149
flow-core/tests/integration/logger/error-handlers.test.ts (1)
1-187: Excellent comprehensive error handling integration test.This test provides thorough validation of the enhanced error handling system with:
- Multiple error types (Error objects and strings)
- Rich LogContext configurations covering all context types
- Context merging functionality testing
- Comprehensive coverage of all optional LogContext fields
The test scenarios effectively validate the integration between error handling and structured logging.
flow-core/src/deps.ts (1)
16-40: LGTM! Excellent dependency centralization for the new architecture.The expansion of exports effectively centralizes key dependencies for:
- RDF and quadstore components (Quadstore, DataFactory, MemoryLevel, Engine)
- JSON-LD types and utilities with proper ESM compatibility
- RDF types namespace for consistent type usage
- Sentry integration for error reporting
This centralization improves dependency management and supports the new quadstore-based configuration system mentioned in the AI summary.
flow-service/src/utils/mesh-utils.ts (1)
1-66: Well-implemented refactoring to async configuration access and structured logging.The changes properly address the previous review comment about async handling. The function is correctly marked as
asyncand usesawaitwhen callingconfig.getMeshPaths(). The enhanced structured logging with operation, component, and metadata fields provides excellent observability for mesh registry initialization.flow-core/docs/error-handling-usage.md (1)
18-18: Verify import path configuration in projectI couldn’t locate any TypeScript path aliases, Deno import maps, webpack aliases, Babel module-resolver settings, or package exports for
@flow/coreor@flow/service. Please confirm that your build/runtime configuration supports these module prefixes—otherwise update the docs or the project’s path mappings.Files & locations to check in docs:
- flow-core/docs/error-handling-usage.md: lines 18, 35
- flow-core/docs/error-handling-usage.md: line 239
flow-core/src/utils/logger/structured-logger.ts (5)
24-42: LGTM! Well-structured logger initialization.The constructor properly handles configuration merging with sensible defaults. Good design choice to enable all channels by default for visibility during development.
107-136: Excellent context scoping implementation.The context creation methods maintain immutability by returning new logger instances. Good use of
crypto.randomUUID()for operation tracking and performance metrics initialization.
241-272: Good defensive programming practices.Excellent use of object spread operators to prevent external mutation of internal state. The ability to update configuration at runtime is useful for dynamic log level adjustments.
274-323: Well-implemented enhanced logger with Error object support.The overloaded method signatures provide flexibility while maintaining backward compatibility. Good use of instanceof for parameter discrimination.
325-349: Clean factory function implementation.Good separation between standard and enhanced logger creation, giving users flexibility to choose based on their error handling needs.
flow-core/src/utils/logger/file-logger.ts (4)
11-26: Well-designed file logger with appropriate defaults.Good encapsulation and reasonable default queue size. The queuing mechanism will help with performance by batching writes.
85-124: Solid daily rotation implementation.Good error handling for missing files and appropriate date comparison logic. The rotated filename format with ISO date is clear and sortable.
125-156: Good utility methods for lifecycle and introspection.The close method properly flushes pending writes, and the getter methods provide useful visibility into logger state.
157-201: Well-implemented singleton pattern with lazy loading.Good handling of async initialization and race conditions by storing the Promise. The reset function will be valuable for testing.
flow-service/src/config/resolution/service-config-resolver.ts (2)
1-24: Good modularization with graph-based configuration approach.The import changes reflect a cleaner separation of concerns with dedicated modules for loading, validation, and access. Enhanced error handling with LogContext improves observability.
89-142: Well-structured CLI option conversion.The function properly handles the transformation from flat CLI options to the nested configuration structure with appropriate JSON-LD type annotations.
flow-core/src/utils/logger/error-handlers.ts (3)
1-15: Clean setup with appropriate logger choice.Good choice using the enhanced logger for error handling to support Error object logging.
16-86: Robust error handling with comprehensive context capture.Excellent defensive programming with unknown error type and fallback console logging. The error context extraction covers all necessary details.
402-466: Excellent utility functions for consistent error handling.The
withErrorHandlingwrapper andcreateContextualErrorHandlerfactory promote consistent error handling patterns across the codebase. Good use of context merging.flow-service/src/utils/service-logger.ts (3)
1-28: Well-structured service context with appropriate defaults.Good use of environment variables with fallbacks. The unique instance ID generation helps with distributed tracing.
59-173: Comprehensive set of context-specific logger factories.Excellent collection of factory functions covering all major service operations. Each provides appropriate contextual information for effective log correlation and debugging.
175-192: Clean utility functions and exports.Good separation between the configured logger instance and utility functions. The default export provides convenient access.
flow-service/src/config/config-types.ts (1)
82-84: Verified:defaultAttributedTois now a string with no residual object usage
All active references to this property have been updated to expect a string:
- flow-service/src/config/config-types.ts
•'fsvc:defaultAttributedTo'?: string
•'conf:defaultAttributedTo'?: string- flow-service/src/services/metadata-composer.ts
• The previousconfig.defaultAttributedToline is commented out and replaced by a hard-coded URI, so no code treats it as an object.- Ripgrep search confirmed no un-commented TypeScript or JSON references expecting an object.
Please ensure any external configuration files are updated to supply a string (e.g. a URI) instead of the former object format.
flow-core/src/utils/logger/formatters.ts (1)
112-152: Well-structured console formatting with environment awarenessThe console formatting function properly adapts output based on the environment, providing colorized output in development and structured JSON in production. The context summarization is concise and informative.
flow-service/src/config/index.ts (1)
90-109: Excellent error handling implementationThe refactored
createServiceConfigfunction properly handles errors with comprehensive context information, making debugging easier. The use of structuredLogContextand thehandleCaughtErrorutility is a best practice.
| return createLogger( | ||
| { | ||
| enableConsole: true, | ||
| enableFile: true, | ||
| enableSentry: true, | ||
| serviceContext: { | ||
| serviceName: appName, | ||
| serviceVersion: appVersion || 'unknown', | ||
| environment: Deno.env.get('DENO_ENV') || 'development', | ||
| }, | ||
| }, | ||
| { | ||
| serviceContext: { | ||
| serviceName: appName, | ||
| serviceVersion: appVersion || 'unknown', | ||
| environment: Deno.env.get('DENO_ENV') || 'development', | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant serviceContext configuration.
Both factory functions pass serviceContext twice - once in the config parameter and once in the baseContext parameter. This duplication appears unnecessary unless the logger implementation specifically requires it in both places.
export function createDefaultLogger(
appName: string,
appVersion?: string,
): StructuredLogger {
return createLogger(
{
enableConsole: true,
enableFile: true,
enableSentry: true,
serviceContext: {
serviceName: appName,
serviceVersion: appVersion || 'unknown',
environment: Deno.env.get('DENO_ENV') || 'development',
},
- },
- {
- serviceContext: {
- serviceName: appName,
- serviceVersion: appVersion || 'unknown',
- environment: Deno.env.get('DENO_ENV') || 'development',
- },
},
);
}Apply the same change to createDefaultEnhancedLogger.
Also applies to: 108-126
🤖 Prompt for AI Agents
In flow-core/src/utils/logger/index.ts around lines 77 to 95 and 108 to 126, the
serviceContext object is redundantly passed in both the config and baseContext
parameters of the createLogger function. Remove the duplicate serviceContext
from the baseContext parameter, keeping it only in the config parameter to avoid
unnecessary duplication. Apply the same removal of redundant serviceContext in
the createDefaultEnhancedLogger function as well.
| serviceName: appName, | ||
| serviceVersion: appVersion || 'unknown', | ||
| environment: Deno.env.get('DENO_ENV') || 'development', | ||
| }, |
There was a problem hiding this comment.
Use consistent environment variable naming.
The code uses DENO_ENV while other parts of the codebase use FLOW_ENV (as mentioned in the formatters.ts snippet). This inconsistency could lead to configuration issues.
- environment: Deno.env.get('DENO_ENV') || 'development',
+ environment: Deno.env.get('FLOW_ENV') || 'development',Apply this change to all four occurrences in both factory functions.
Also applies to: 93-93, 116-116, 123-123
🤖 Prompt for AI Agents
In flow-core/src/utils/logger/index.ts at lines 86, 93, 116, and 123, replace
all occurrences of the environment variable name DENO_ENV with FLOW_ENV to
maintain consistent naming across the codebase. Update both factory functions
accordingly to ensure configuration uses FLOW_ENV uniformly.
| console.log(`🔧 Sentry enabled in config: ${sentryEnabled}`); | ||
|
|
||
| if (sentryEnabled && !sentryInitialized) { | ||
| console.log(`🔧 Initializing Sentry...`); | ||
| try { | ||
| Sentry.init({ | ||
| dsn: sentryDsn, | ||
| environment: config?.environment || (isDevelopment ? 'development' : 'production'), | ||
| debug: config?.debug ?? isDevelopment, | ||
| tracesSampleRate: config?.tracesSampleRate ?? (isDevelopment ? 1.0 : 0.1), | ||
| release: config?.release || Deno.env.get('FLOW_VERSION'), | ||
| // Enable logs to be sent to Sentry (experimental feature) | ||
| _experiments: { | ||
| enableLogs: config?.enableLogs ?? true | ||
| }, | ||
| }); | ||
| sentryInitialized = true; | ||
| console.log(`🔧 DEBUG: Sentry initialized successfully`); | ||
| } catch (error) { | ||
| // Use console.error here to avoid circular dependency with error handlers | ||
| console.error(`🔧 ERROR: Failed to initialize Sentry: ${error}`); | ||
| console.error(`🔧 ERROR: Sentry will remain disabled`); | ||
| } | ||
| } else if (Deno.env.get('FLOW_SENTRY_ENABLED') !== 'true') { | ||
| console.log(`🔧 Sentry disabled - FLOW_SENTRY_ENABLED is not 'true'`); | ||
| } else if (!sentryDsn) { | ||
| console.log(`🔧 Sentry disabled - no DSN provided`); | ||
| } else { | ||
| console.log(`🔧 Sentry already initialized, skipping`); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace console.log with proper logging mechanism
The initialization function uses console.log statements for debugging output. These should be replaced with the structured logger to maintain consistency and allow proper log level control.
Consider using the structured logger instead of console.log for initialization messages. This would allow proper log level control and consistent formatting across the application.
- console.log(`🔧 Sentry enabled in config: ${sentryEnabled}`);
+ // Use structured logger once available
+ // logger.debug('Sentry enabled in config', { sentryEnabled });📝 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.
| console.log(`🔧 Sentry enabled in config: ${sentryEnabled}`); | |
| if (sentryEnabled && !sentryInitialized) { | |
| console.log(`🔧 Initializing Sentry...`); | |
| try { | |
| Sentry.init({ | |
| dsn: sentryDsn, | |
| environment: config?.environment || (isDevelopment ? 'development' : 'production'), | |
| debug: config?.debug ?? isDevelopment, | |
| tracesSampleRate: config?.tracesSampleRate ?? (isDevelopment ? 1.0 : 0.1), | |
| release: config?.release || Deno.env.get('FLOW_VERSION'), | |
| // Enable logs to be sent to Sentry (experimental feature) | |
| _experiments: { | |
| enableLogs: config?.enableLogs ?? true | |
| }, | |
| }); | |
| sentryInitialized = true; | |
| console.log(`🔧 DEBUG: Sentry initialized successfully`); | |
| } catch (error) { | |
| // Use console.error here to avoid circular dependency with error handlers | |
| console.error(`🔧 ERROR: Failed to initialize Sentry: ${error}`); | |
| console.error(`🔧 ERROR: Sentry will remain disabled`); | |
| } | |
| } else if (Deno.env.get('FLOW_SENTRY_ENABLED') !== 'true') { | |
| console.log(`🔧 Sentry disabled - FLOW_SENTRY_ENABLED is not 'true'`); | |
| } else if (!sentryDsn) { | |
| console.log(`🔧 Sentry disabled - no DSN provided`); | |
| } else { | |
| console.log(`🔧 Sentry already initialized, skipping`); | |
| // Use structured logger once available | |
| // logger.debug('Sentry enabled in config', { sentryEnabled }); | |
| if (sentryEnabled && !sentryInitialized) { | |
| console.log(`🔧 Initializing Sentry...`); | |
| try { | |
| Sentry.init({ | |
| dsn: sentryDsn, | |
| environment: config?.environment || (isDevelopment ? 'development' : 'production'), | |
| debug: config?.debug ?? isDevelopment, | |
| tracesSampleRate: config?.tracesSampleRate ?? (isDevelopment ? 1.0 : 0.1), | |
| release: config?.release || Deno.env.get('FLOW_VERSION'), | |
| // Enable logs to be sent to Sentry (experimental feature) | |
| _experiments: { | |
| enableLogs: config?.enableLogs ?? true | |
| }, | |
| }); | |
| sentryInitialized = true; | |
| console.log(`🔧 DEBUG: Sentry initialized successfully`); | |
| } catch (error) { | |
| // Use console.error here to avoid circular dependency with error handlers | |
| console.error(`🔧 ERROR: Failed to initialize Sentry: ${error}`); | |
| console.error(`🔧 ERROR: Sentry will remain disabled`); | |
| } | |
| } else if (Deno.env.get('FLOW_SENTRY_ENABLED') !== 'true') { | |
| console.log(`🔧 Sentry disabled - FLOW_SENTRY_ENABLED is not 'true'`); | |
| } else if (!sentryDsn) { | |
| console.log(`🔧 Sentry disabled - no DSN provided`); | |
| } else { | |
| console.log(`🔧 Sentry already initialized, skipping`); |
🤖 Prompt for AI Agents
In flow-core/src/utils/logger/sentry-logger.ts between lines 44 and 72, replace
all console.log calls used for Sentry initialization status messages with the
structured logger used elsewhere in the application. This ensures consistent log
formatting and enables proper log level control. Also replace console.error
calls with the structured logger's error method to maintain uniform error
handling and avoid direct console usage.
| // Enable logs to be sent to Sentry (experimental feature) | ||
| _experiments: { | ||
| enableLogs: config?.enableLogs ?? true | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify experimental feature stability
The code enables an experimental Sentry feature (enableLogs). Experimental features may change or be removed in future versions.
🌐 Web query:
What is the current status of the Sentry _experiments.enableLogs feature and is it stable for production use?
💡 Result:
Sentry's _experiments.enableLogs feature, which enables structured logging, has evolved over time. Initially introduced as an experimental feature, it has since been integrated into the main SDK with a more stable configuration.
Current Status:
As of version 9.41.0, the enableLogs feature is no longer under the _experiments namespace. It has been moved to the top-level configuration, indicating a more stable and production-ready status. (gitlab.com)
Stability for Production Use:
The transition from _experiments.enableLogs to enableLogs suggests that the feature is now considered stable for production environments. Sentry's official documentation and recent releases support this, indicating that the structured logging feature is ready for use in production settings. (docs.sentry.dev)
Implementation:
To enable structured logging in your Sentry configuration, you can initialize the SDK as follows:
Sentry.init({
dsn: 'YOUR_PUBLIC_DSN',
enableLogs: true,
});This setup allows you to send, view, and query logs within Sentry, providing a unified debugging experience alongside error and performance monitoring. (docs.sentry.dev)
In summary, the enableLogs feature has been integrated into Sentry's main SDK and is now stable for production use, offering enhanced logging capabilities for your applications.
Citations:
- 1: https://gitlab.com/gitlab-org/cells/http-router/-/merge_requests/818?utm_source=openai
- 2: https://docs.sentry.dev/platforms/javascript/guides/connect/logs/?utm_source=openai
- 3: https://docs.sentry.dev/platforms/javascript/guides/connect/logs/?utm_source=openai
Update Sentry logging configuration to use the stable enableLogs option
Sentry has promoted enableLogs out of the _experiments namespace as of v9.41.0, so you no longer need to pass it under _experiments. Please update your initializer accordingly.
• File: flow-core/src/utils/logger/sentry-logger.ts
Replace the _experiments block with a top-level enableLogs setting.
Suggested diff:
--- a/flow-core/src/utils/logger/sentry-logger.ts
+++ b/flow-core/src/utils/logger/sentry-logger.ts
@@ lines 55-58
- // Enable logs to be sent to Sentry (experimental feature)
- _experiments: {
- enableLogs: config?.enableLogs ?? true
- },
+ // Enable structured logs in Sentry (now stable in SDK ≥9.41.0)
+ enableLogs: config?.enableLogs ?? true,This aligns with the current Sentry SDK usage:
Sentry.init({
dsn: 'YOUR_PUBLIC_DSN',
enableLogs: true, // production-ready
});🤖 Prompt for AI Agents
In flow-core/src/utils/logger/sentry-logger.ts around lines 55 to 58, the Sentry
initialization incorrectly places the enableLogs option inside the _experiments
object. Remove the _experiments block and move enableLogs to be a top-level
property in the Sentry.init configuration object, setting it directly to
config?.enableLogs or true as default.
| async debug( | ||
| message: string, | ||
| context?: LogContext, | ||
| meta?: Record<string, unknown>, | ||
| ): Promise<void> { | ||
| if (!shouldLog('DEBUG')) return; | ||
| const mergedContext = this.mergeContexts(context, meta); | ||
| await this.writeToAllChannels('DEBUG', message, mergedContext); | ||
| } | ||
|
|
||
| /** | ||
| * Log informational messages | ||
| */ | ||
| async info( | ||
| message: string, | ||
| context?: LogContext, | ||
| meta?: Record<string, unknown>, | ||
| ): Promise<void> { | ||
| if (!shouldLog('INFO')) return; | ||
| const mergedContext = this.mergeContexts(context, meta); | ||
| await this.writeToAllChannels('INFO', message, mergedContext); | ||
| } | ||
|
|
||
| /** | ||
| * Log warning messages | ||
| */ | ||
| async warn( | ||
| message: string, | ||
| context?: LogContext, | ||
| meta?: Record<string, unknown>, | ||
| ): Promise<void> { | ||
| if (!shouldLog('WARN')) return; | ||
| const mergedContext = this.mergeContexts(context, meta); | ||
| await this.writeToAllChannels('WARN', message, mergedContext); | ||
| } | ||
|
|
||
| /** | ||
| * Log error messages | ||
| */ | ||
| async error( | ||
| message: string, | ||
| context?: LogContext, | ||
| meta?: Record<string, unknown>, | ||
| ): Promise<void> { | ||
| if (!shouldLog('ERROR')) return; | ||
| const mergedContext = this.mergeContexts(context, meta); | ||
| await this.writeToAllChannels('ERROR', message, mergedContext); | ||
| } | ||
|
|
||
| /** | ||
| * Log critical error messages | ||
| */ | ||
| async critical( | ||
| message: string, | ||
| context?: LogContext, | ||
| meta?: Record<string, unknown>, | ||
| ): Promise<void> { | ||
| const mergedContext = this.mergeContexts(context, meta); | ||
| await this.writeToAllChannels('CRITICAL', message, mergedContext); | ||
| } |
There was a problem hiding this comment.
Fix async/sync method signature mismatch with interface.
The logging methods are declared as async and return Promise<void>, but according to the StructuredLogger interface in types.ts, they should return void. This mismatch could cause TypeScript compilation errors.
Either update the interface to reflect async methods or make these methods synchronous by handling async operations internally without awaiting.
🤖 Prompt for AI Agents
In flow-core/src/utils/logger/structured-logger.ts between lines 46 and 105, the
logging methods are declared async and return Promise<void>, but the
StructuredLogger interface expects them to return void, causing a type mismatch.
To fix this, either update the StructuredLogger interface in types.ts to have
these methods return Promise<void> to match the async implementation, or
refactor these methods to be synchronous by removing async/await and handling
any asynchronous operations internally without returning a Promise.
| export interface StructuredLogger { | ||
| /** | ||
| * Log debug information | ||
| * @param message - The log message | ||
| * @param context - Optional logging context | ||
| * @param meta - Additional metadata | ||
| */ | ||
| debug(message: string, context?: LogContext, meta?: Record<string, unknown>): void; | ||
|
|
||
| /** | ||
| * Log informational messages | ||
| * @param message - The log message | ||
| * @param context - Optional logging context | ||
| * @param meta - Additional metadata | ||
| */ | ||
| info(message: string, context?: LogContext, meta?: Record<string, unknown>): void; | ||
|
|
||
| /** | ||
| * Log warning messages | ||
| * @param message - The log message | ||
| * @param context - Optional logging context | ||
| * @param meta - Additional metadata | ||
| */ | ||
| warn(message: string, context?: LogContext, meta?: Record<string, unknown>): void; | ||
|
|
||
| /** | ||
| * Log error messages | ||
| * @param message - The log message | ||
| * @param context - Optional logging context | ||
| * @param meta - Additional metadata | ||
| */ | ||
| error(message: string, context?: LogContext, meta?: Record<string, unknown>): void; | ||
|
|
||
| /** | ||
| * Log critical error messages | ||
| * @param message - The log message | ||
| * @param context - Optional logging context | ||
| * @param meta - Additional metadata | ||
| */ | ||
| critical(message: string, context?: LogContext, meta?: Record<string, unknown>): void; |
There was a problem hiding this comment.
Update StructuredLogger interface to reflect async implementation.
The interface methods are defined as returning void, but according to the implementation in structured-logger.ts, these methods are async and return Promise<void>. This type mismatch will cause issues when using the logger.
export interface StructuredLogger {
/**
* Log debug information
* @param message - The log message
* @param context - Optional logging context
* @param meta - Additional metadata
*/
- debug(message: string, context?: LogContext, meta?: Record<string, unknown>): void;
+ debug(message: string, context?: LogContext, meta?: Record<string, unknown>): Promise<void>;
/**
* Log informational messages
* @param message - The log message
* @param context - Optional logging context
* @param meta - Additional metadata
*/
- info(message: string, context?: LogContext, meta?: Record<string, unknown>): void;
+ info(message: string, context?: LogContext, meta?: Record<string, unknown>): Promise<void>;
/**
* Log warning messages
* @param message - The log message
* @param context - Optional logging context
* @param meta - Additional metadata
*/
- warn(message: string, context?: LogContext, meta?: Record<string, unknown>): void;
+ warn(message: string, context?: LogContext, meta?: Record<string, unknown>): Promise<void>;
/**
* Log error messages
* @param message - The log message
* @param context - Optional logging context
* @param meta - Additional metadata
*/
- error(message: string, context?: LogContext, meta?: Record<string, unknown>): void;
+ error(message: string, context?: LogContext, meta?: Record<string, unknown>): Promise<void>;
/**
* Log critical error messages
* @param message - The log message
* @param context - Optional logging context
* @param meta - Additional metadata
*/
- critical(message: string, context?: LogContext, meta?: Record<string, unknown>): void;
+ critical(message: string, context?: LogContext, meta?: Record<string, unknown>): Promise<void>;📝 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.
| export interface StructuredLogger { | |
| /** | |
| * Log debug information | |
| * @param message - The log message | |
| * @param context - Optional logging context | |
| * @param meta - Additional metadata | |
| */ | |
| debug(message: string, context?: LogContext, meta?: Record<string, unknown>): void; | |
| /** | |
| * Log informational messages | |
| * @param message - The log message | |
| * @param context - Optional logging context | |
| * @param meta - Additional metadata | |
| */ | |
| info(message: string, context?: LogContext, meta?: Record<string, unknown>): void; | |
| /** | |
| * Log warning messages | |
| * @param message - The log message | |
| * @param context - Optional logging context | |
| * @param meta - Additional metadata | |
| */ | |
| warn(message: string, context?: LogContext, meta?: Record<string, unknown>): void; | |
| /** | |
| * Log error messages | |
| * @param message - The log message | |
| * @param context - Optional logging context | |
| * @param meta - Additional metadata | |
| */ | |
| error(message: string, context?: LogContext, meta?: Record<string, unknown>): void; | |
| /** | |
| * Log critical error messages | |
| * @param message - The log message | |
| * @param context - Optional logging context | |
| * @param meta - Additional metadata | |
| */ | |
| critical(message: string, context?: LogContext, meta?: Record<string, unknown>): void; | |
| export interface StructuredLogger { | |
| /** | |
| * Log debug information | |
| * @param message - The log message | |
| * @param context - Optional logging context | |
| * @param meta - Additional metadata | |
| */ | |
| debug(message: string, context?: LogContext, meta?: Record<string, unknown>): Promise<void>; | |
| /** | |
| * Log informational messages | |
| * @param message - The log message | |
| * @param context - Optional logging context | |
| * @param meta - Additional metadata | |
| */ | |
| info(message: string, context?: LogContext, meta?: Record<string, unknown>): Promise<void>; | |
| /** | |
| * Log warning messages | |
| * @param message - The log message | |
| * @param context - Optional logging context | |
| * @param meta - Additional metadata | |
| */ | |
| warn(message: string, context?: LogContext, meta?: Record<string, unknown>): Promise<void>; | |
| /** | |
| * Log error messages | |
| * @param message - The log message | |
| * @param context - Optional logging context | |
| * @param meta - Additional metadata | |
| */ | |
| error(message: string, context?: LogContext, meta?: Record<string, unknown>): Promise<void>; | |
| /** | |
| * Log critical error messages | |
| * @param message - The log message | |
| * @param context - Optional logging context | |
| * @param meta - Additional metadata | |
| */ | |
| critical(message: string, context?: LogContext, meta?: Record<string, unknown>): Promise<void>; | |
| } |
🤖 Prompt for AI Agents
In flow-core/src/utils/logger/types.ts between lines 84 and 123, the
StructuredLogger interface methods are currently typed to return void, but the
actual implementation uses async methods returning Promise<void>. Update the
return types of all logging methods (debug, info, warn, error, critical) to
return Promise<void> to match the async implementation and avoid type
mismatches.
| const structuredMsg = formatStructuredMessage('DEBUG', 'Processing mesh nodes', complexContext); | ||
| console.log('Formatted structured message:', JSON.stringify(structuredMsg, null, 2)); |
There was a problem hiding this comment.
Remove redundant JSON.stringify on already stringified JSON.
The formatStructuredMessage function already returns a JSON string. Calling JSON.stringify on it will double-encode the JSON, resulting in escaped quotes. To properly display the formatted JSON, parse it first:
const structuredMsg = formatStructuredMessage('DEBUG', 'Processing mesh nodes', complexContext);
-console.log('Formatted structured message:', JSON.stringify(structuredMsg, null, 2));
+console.log('Formatted structured message:', JSON.stringify(JSON.parse(structuredMsg), null, 2));Alternatively, just log the string directly:
-console.log('Formatted structured message:', JSON.stringify(structuredMsg, null, 2));
+console.log('Formatted structured message:', structuredMsg);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const structuredMsg = formatStructuredMessage('DEBUG', 'Processing mesh nodes', complexContext); | |
| console.log('Formatted structured message:', JSON.stringify(structuredMsg, null, 2)); | |
| const structuredMsg = formatStructuredMessage('DEBUG', 'Processing mesh nodes', complexContext); | |
| console.log('Formatted structured message:', JSON.stringify(JSON.parse(structuredMsg), null, 2)); |
🤖 Prompt for AI Agents
In flow-core/tests/integration/logger/formatters.test.ts at lines 52-53, the
code calls JSON.stringify on the result of formatStructuredMessage, which
already returns a JSON string, causing double encoding. Fix this by either
logging the string directly without JSON.stringify or by parsing the string
first with JSON.parse before logging to display the formatted JSON properly.
| readonly 'fsvc:logRotationInterval'?: | ||
| | 'daily' | ||
| | 'weekly' | ||
| | 'monthly' | ||
| | 'size-based'; | ||
| | 'daily' | ||
| | 'weekly' | ||
| | 'monthly' | ||
| | 'size-based'; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify consistent naming convention for rotation intervals
The log rotation interval values use kebab-case format. Ensure this naming convention is consistently handled throughout the codebase, especially in any parsing or validation logic.
🏁 Script executed:
#!/bin/bash
# Description: Check for consistent handling of log rotation interval values
# Search for references to log rotation intervals
rg -A 3 "logRotationInterval|rotation.*interval" --type tsLength of output: 1433
Include all interval options in loader and validate inputs
The union in config-types.ts now includes size-based, but the env-loader only asserts
'daily' | 'weekly' | 'monthly'. Update it to cover size-based, and add a runtime check to reject invalid values.
• File: flow-service/src/config/loaders/env-loader.ts
– Change the type assertion to:
ts fileChannel['fsvc:logRotationInterval'] = interval as | 'daily' | 'weekly' | 'monthly' | 'size-based';
– Before assigning, verify:
ts const validIntervals = ['daily', 'weekly', 'monthly', 'size-based'] as const; if (!validIntervals.includes(interval as any)) { throw new Error( `Invalid logRotationInterval "${interval}". Must be one of ${validIntervals.join(', ')}` ); }
• File: flow-service/src/config/defaults.ts
– Consider allowing other defaults beyond daily, or document why only daily is used.
This ensures the kebab-case convention (including size-based) is consistently parsed and validated.
🤖 Prompt for AI Agents
In flow-service/src/config/loaders/env-loader.ts around lines where
fsvc:logRotationInterval is assigned, update the type assertion to include
'size-based' along with 'daily', 'weekly', and 'monthly'. Before assigning, add
a runtime validation that checks if the interval value is one of these valid
options; if not, throw an error indicating the invalid value and listing the
allowed options. Also, review flow-service/src/config/defaults.ts to either
allow other default values beyond 'daily' or add documentation explaining why
'daily' is the only default used.
| /** | ||
| * Service-specific logger configuration | ||
| */ | ||
| const SERVICE_LOGGER_CONFIG: LoggerConfig = { | ||
| enableConsole: true, | ||
| enableFile: Deno.env.get('FLOW_LOG_FILE_ENABLED') === 'true', | ||
| enableSentry: Deno.env.get('FLOW_SENTRY_ENABLED') === 'true', | ||
|
|
||
| fileConfig: { | ||
| logDir: Deno.env.get('FLOW_LOG_DIR') || './logs', | ||
| maxFileSize: parseInt(Deno.env.get('FLOW_LOG_MAX_FILE_SIZE') || '10485760'), // 10MB | ||
| maxFiles: parseInt(Deno.env.get('FLOW_LOG_MAX_FILES') || '5'), | ||
| rotateDaily: Deno.env.get('FLOW_LOG_ROTATE_DAILY') === 'true', | ||
| }, | ||
|
|
||
| sentryConfig: { | ||
| dsn: Deno.env.get('FLOW_SENTRY_DSN'), | ||
| environment: Deno.env.get('FLOW_ENV') || 'development', | ||
| release: Deno.env.get('FLOW_VERSION'), | ||
| sampleRate: parseFloat(Deno.env.get('FLOW_SENTRY_SAMPLE_RATE') || '1.0'), | ||
| }, | ||
|
|
||
| serviceContext: SERVICE_CONTEXT, | ||
| }; |
There was a problem hiding this comment.
Add validation for numeric environment variable parsing.
The parseInt and parseFloat calls could produce NaN values if environment variables contain invalid values, which could cause runtime errors.
const SERVICE_LOGGER_CONFIG: LoggerConfig = {
enableConsole: true,
enableFile: Deno.env.get('FLOW_LOG_FILE_ENABLED') === 'true',
enableSentry: Deno.env.get('FLOW_SENTRY_ENABLED') === 'true',
fileConfig: {
logDir: Deno.env.get('FLOW_LOG_DIR') || './logs',
- maxFileSize: parseInt(Deno.env.get('FLOW_LOG_MAX_FILE_SIZE') || '10485760'), // 10MB
- maxFiles: parseInt(Deno.env.get('FLOW_LOG_MAX_FILES') || '5'),
+ maxFileSize: parseInt(Deno.env.get('FLOW_LOG_MAX_FILE_SIZE') || '10485760') || 10485760, // 10MB
+ maxFiles: parseInt(Deno.env.get('FLOW_LOG_MAX_FILES') || '5') || 5,
rotateDaily: Deno.env.get('FLOW_LOG_ROTATE_DAILY') === 'true',
},
sentryConfig: {
dsn: Deno.env.get('FLOW_SENTRY_DSN'),
environment: Deno.env.get('FLOW_ENV') || 'development',
release: Deno.env.get('FLOW_VERSION'),
- sampleRate: parseFloat(Deno.env.get('FLOW_SENTRY_SAMPLE_RATE') || '1.0'),
+ sampleRate: parseFloat(Deno.env.get('FLOW_SENTRY_SAMPLE_RATE') || '1.0') || 1.0,
},
serviceContext: SERVICE_CONTEXT,
};📝 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.
| /** | |
| * Service-specific logger configuration | |
| */ | |
| const SERVICE_LOGGER_CONFIG: LoggerConfig = { | |
| enableConsole: true, | |
| enableFile: Deno.env.get('FLOW_LOG_FILE_ENABLED') === 'true', | |
| enableSentry: Deno.env.get('FLOW_SENTRY_ENABLED') === 'true', | |
| fileConfig: { | |
| logDir: Deno.env.get('FLOW_LOG_DIR') || './logs', | |
| maxFileSize: parseInt(Deno.env.get('FLOW_LOG_MAX_FILE_SIZE') || '10485760'), // 10MB | |
| maxFiles: parseInt(Deno.env.get('FLOW_LOG_MAX_FILES') || '5'), | |
| rotateDaily: Deno.env.get('FLOW_LOG_ROTATE_DAILY') === 'true', | |
| }, | |
| sentryConfig: { | |
| dsn: Deno.env.get('FLOW_SENTRY_DSN'), | |
| environment: Deno.env.get('FLOW_ENV') || 'development', | |
| release: Deno.env.get('FLOW_VERSION'), | |
| sampleRate: parseFloat(Deno.env.get('FLOW_SENTRY_SAMPLE_RATE') || '1.0'), | |
| }, | |
| serviceContext: SERVICE_CONTEXT, | |
| }; | |
| /** | |
| * Service-specific logger configuration | |
| */ | |
| const SERVICE_LOGGER_CONFIG: LoggerConfig = { | |
| enableConsole: true, | |
| enableFile: Deno.env.get('FLOW_LOG_FILE_ENABLED') === 'true', | |
| enableSentry: Deno.env.get('FLOW_SENTRY_ENABLED') === 'true', | |
| fileConfig: { | |
| logDir: Deno.env.get('FLOW_LOG_DIR') || './logs', | |
| maxFileSize: parseInt(Deno.env.get('FLOW_LOG_MAX_FILE_SIZE') || '10485760') || 10485760, // 10MB | |
| maxFiles: parseInt(Deno.env.get('FLOW_LOG_MAX_FILES') || '5') || 5, | |
| rotateDaily: Deno.env.get('FLOW_LOG_ROTATE_DAILY') === 'true', | |
| }, | |
| sentryConfig: { | |
| dsn: Deno.env.get('FLOW_SENTRY_DSN'), | |
| environment: Deno.env.get('FLOW_ENV') || 'development', | |
| release: Deno.env.get('FLOW_VERSION'), | |
| sampleRate: parseFloat(Deno.env.get('FLOW_SENTRY_SAMPLE_RATE') || '1.0') || 1.0, | |
| }, | |
| serviceContext: SERVICE_CONTEXT, | |
| }; |
🤖 Prompt for AI Agents
In flow-service/src/utils/service-logger.ts between lines 29 and 52, the numeric
environment variables parsed with parseInt and parseFloat can result in NaN if
the input is invalid. Add validation checks after parsing these values to ensure
they are valid numbers; if not, fallback to default values to prevent runtime
errors. Implement this validation for maxFileSize, maxFiles, and sampleRate
fields in the SERVICE_LOGGER_CONFIG object.
…vironment variable handling
- Added `expandRelativeIds` function to expand relative IDs in JSON-LD objects based on a provided base IRI. - Updated `jsonldToQuads` function to utilize `jsonld.toRDF` for converting JSON-LD to RDFJS quads. - Modified `flow-service-config.jsonld` to adjust service configuration, including changes to ports and rights holders. - Refactored `config-types.ts` to standardize property names and types across interfaces. - Updated default service configurations in `defaults.ts` to reflect new structure and IDs. - Enhanced environment variable loading in `env-loader.ts` to include new service configuration properties. - Implemented dynamic URI handling in `quadstore-loader.ts` for loading platform defaults and input configurations. - Adjusted service configuration resolution logic in `service-config-resolver.ts` to utilize updated loading functions. - Cleaned up service URI building logic in `service-uri-builder.ts` to simplify graph URI construction.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
flow-service/src/config/loaders/quadstore-loader.ts (1)
11-17: Consider adding error handling for platform defaults loading.The function should handle potential errors when loading platform defaults, as failures here could prevent the entire configuration system from initializing.
export async function loadPlatformServiceDefaults(): Promise<void> { - - const uri = getCurrentServiceUri(CONFIG_GRAPH_NAMES.platformServiceDefaults); - const expandedPlatformServiceDefaults = expandRelativeIds(PLATFORM_SERVICE_DEFAULTS, uri); - // use the Service URI for the graph name - await createNewGraphFromJsonLd(expandedPlatformServiceDefaults, { graphName: uri }); + try { + const uri = getCurrentServiceUri(CONFIG_GRAPH_NAMES.platformServiceDefaults); + const expandedPlatformServiceDefaults = expandRelativeIds(PLATFORM_SERVICE_DEFAULTS, uri); + // use the Service URI for the graph name + await createNewGraphFromJsonLd(expandedPlatformServiceDefaults, { graphName: uri }); + } catch (error) { + throw new Error(`Failed to load platform service defaults: ${error instanceof Error ? error.message : String(error)}`); + } }
🧹 Nitpick comments (7)
flow-core/src/utils/rdfjs-utils.ts (1)
14-16: Clean up blank lines and confirm jsonld.toRDF return typeRemoved two empty lines after the function signature and added a note to ensure the return value really is an array of
RDF.Quad.• Apply this diff to remove the blank lines:
): Promise<RDF.Quad[]> { - - const quads = await jsonld.toRDF(inputJsonLd) as RDF.Quad[];• Verify that
jsonld.toRDF(inputJsonLd)actually returnsRDF.Quad[](per the jsonld.js API), or adjust the call/options (e.g. specify{format: 'application/n-quads'}or update the type definitions) so that the assertion is guaranteed safe.flow-service/src/utils/service-uri-builder.ts (1)
31-35: Consider handling trailing slashes more carefully.The function always appends a trailing slash, which might not be appropriate for all paths (e.g., file paths or query parameters).
Consider making trailing slash addition conditional:
export function buildServiceUri(config: ServiceUriConfig, path: string): string { const baseUri = buildServiceBaseUri(config); const cleanPath = path.replace(/^\/+/, ''); - return `${baseUri}${cleanPath}/`; + // Only add trailing slash if path doesn't end with one and doesn't contain query/fragment + const needsTrailingSlash = cleanPath && !cleanPath.endsWith('/') && + !cleanPath.includes('?') && !cleanPath.includes('#'); + return `${baseUri}${cleanPath}${needsTrailingSlash ? '/' : ''}`; }flow-service/src/config/loaders/quadstore-loader.ts (1)
69-76: Remove console.log and clean up empty lines.The function contains a console.log statement that should use proper logging, and has unnecessary empty lines.
if (exists === 0) { - console.log(`platform quad copied: ${q.subject.value} ${q.predicate.value} ${q.object.value} in graph ${mergedUri}`); - await store.put(df.quad(q.subject, q.predicate, q.object, df.namedNode(mergedUri))); } } - - }If logging is needed, consider using the service logger instead of console.log.
flow-service/src/config/resolution/service-config-resolver.ts (2)
37-37: Remove commented debugging code.The commented console.log statement should be removed.
// Load environment config const envConfig = loadEnvConfig(); - //console.log(envConfig) // Load file config if specified
67-69: Track TODO for loading mesh node configuration.The TODO comment indicates missing functionality for loading input mesh root node configuration.
Would you like me to create an issue to track the implementation of mesh root node configuration loading?
flow-service/src/config/config-types.ts (1)
60-61: Clarify or complete the TODO for type renaming.The TODO comment is incomplete. What should
MeshRootNodeConfigbe renamed to?Would you like me to help determine an appropriate name based on the type's usage pattern, or should this TODO be removed if no longer relevant?
flow-service/src/config/index.ts (1)
10-20: Consider using 'as const' for better type safetyThe graph names constant would benefit from const assertion to ensure type safety and prevent accidental modifications.
-export const CONFIG_GRAPH_NAMES = { +export const CONFIG_GRAPH_NAMES = { platformServiceDefaults: 'platformServiceDefaults', platformImplicitMeshRootNodeConfig: 'platformImplicitMeshRootNodeConfig', inputServiceConfig: 'inputServiceConfig', inputMeshRootNodeConfig: 'inputMeshRootNodeConfig', mergedServiceConfig: 'mergedServiceConfig', -} +} as const;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
flow-core/src/utils/quadstore-utils.ts(1 hunks)flow-core/src/utils/rdfjs-utils.ts(1 hunks)flow-service/flow-service-config.jsonld(1 hunks)flow-service/main.ts(6 hunks)flow-service/src/config/config-types.ts(1 hunks)flow-service/src/config/defaults.ts(2 hunks)flow-service/src/config/index.ts(2 hunks)flow-service/src/config/loaders/env-loader.ts(12 hunks)flow-service/src/config/loaders/quadstore-loader.ts(1 hunks)flow-service/src/config/resolution/service-config-accessor.ts(1 hunks)flow-service/src/config/resolution/service-config-loader.ts(1 hunks)flow-service/src/config/resolution/service-config-resolver.ts(2 hunks)flow-service/src/config/resolution/service-config-validator.ts(1 hunks)flow-service/src/service-constants.ts(1 hunks)flow-service/src/utils/service-log-context.ts(1 hunks)flow-service/src/utils/service-uri-builder.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- flow-service/src/service-constants.ts
- flow-service/src/utils/service-log-context.ts
- flow-service/flow-service-config.jsonld
🚧 Files skipped from review as they are similar to previous changes (7)
- flow-service/src/config/resolution/service-config-validator.ts
- flow-service/main.ts
- flow-service/src/config/resolution/service-config-loader.ts
- flow-service/src/config/resolution/service-config-accessor.ts
- flow-service/src/config/defaults.ts
- flow-core/src/utils/quadstore-utils.ts
- flow-service/src/config/loaders/env-loader.ts
🔇 Additional comments (11)
flow-service/src/utils/service-uri-builder.ts (2)
9-26: LGTM! Well-structured URI configuration and builder.The interface and base URI builder correctly handle default ports for HTTP/HTTPS protocols.
41-73: LGTM! Well-implemented singleton with proper state management.The singleton correctly manages configuration state, implements caching for performance, and provides clear error messages when accessed before initialization.
flow-service/src/config/resolution/service-config-resolver.ts (1)
106-155: LGTM! Well-structured CLI options conversion.The function correctly maps all CLI options to the configuration format, including proper validation of log levels and nested configuration object construction.
flow-service/src/config/config-types.ts (2)
171-187: LGTM! Well-designed error classes.The error classes properly extend Error, use descriptive names, and support error chaining through the cause property. The ConfigValidationError appropriately extends ConfigError with additional validation error details.
31-36: Env-loader supports the new “size-based” optionEnv-loader’s assignment now casts
intervalto all four variants—including"size-based"—so no further changes are needed.flow-service/src/config/index.ts (6)
8-9: LGTM!The import aligns with the new structured logging system.
23-36: LGTM!Type exports properly reflect the new "mesh root node" terminology and modular structure.
39-39: LGTM!Error class exports support proper typed error handling.
60-65: LGTM!Function renames are consistent with the new mesh root node terminology and follow proper naming conventions.
72-75: LGTM!The modular exports properly separate validation and configuration access concerns.
80-108: Error handling utilities verified
handleCaughtErroris exported as an async function inflow-core/src/utils/logger/error-handlers.ts(line 98).LogContextinterface is defined and exported inflow-core/src/utils/logger/types.ts(line 11).No further actions required. Approving the changes.
| if (typeof obj["@id"] === "string" && !obj["@id"].startsWith("http")) { | ||
| obj["@id"] = baseIRI + obj["@id"]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve URL validation for @id fields.
The current check !obj["@id"].startsWith("http") is too broad and could match invalid schemes like "httpx://". Consider checking for valid URL schemes more precisely.
Apply this diff to improve the validation:
- if (typeof obj["@id"] === "string" && !obj["@id"].startsWith("http")) {
+ if (typeof obj["@id"] === "string" && !obj["@id"].startsWith("http://") && !obj["@id"].startsWith("https://")) {
obj["@id"] = baseIRI + obj["@id"];
}📝 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.
| if (typeof obj["@id"] === "string" && !obj["@id"].startsWith("http")) { | |
| obj["@id"] = baseIRI + obj["@id"]; | |
| } | |
| if ( | |
| typeof obj["@id"] === "string" && | |
| !obj["@id"].startsWith("http://") && | |
| !obj["@id"].startsWith("https://") | |
| ) { | |
| obj["@id"] = baseIRI + obj["@id"]; | |
| } |
🤖 Prompt for AI Agents
In flow-core/src/utils/rdfjs-utils.ts around lines 42 to 44, the current check
for obj["@id"] uses startsWith("http"), which can incorrectly match invalid
schemes like "httpx://". To fix this, replace the check with a more precise
validation that ensures obj["@id"] starts specifically with "http://" or
"https://". Update the condition to verify the URL scheme explicitly before
deciding to prepend baseIRI.
| export async function loadPlatformImplicitMeshRootNodeConfig(): Promise<void> { | ||
| const uri = getCurrentServiceUri(CONFIG_GRAPH_NAMES.platformImplicitMeshRootNodeConfig); | ||
| const expandedPlatformNodeDefaults = expandRelativeIds(PLATFORM_NODE_DEFAULTS, uri); | ||
| await createNewGraphFromJsonLd(expandedPlatformNodeDefaults, { graphName: uri }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for platform mesh root node config loading.
Similar to the previous function, this should handle potential errors during loading.
export async function loadPlatformImplicitMeshRootNodeConfig(): Promise<void> {
- const uri = getCurrentServiceUri(CONFIG_GRAPH_NAMES.platformImplicitMeshRootNodeConfig);
- const expandedPlatformNodeDefaults = expandRelativeIds(PLATFORM_NODE_DEFAULTS, uri);
- await createNewGraphFromJsonLd(expandedPlatformNodeDefaults, { graphName: uri });
+ try {
+ const uri = getCurrentServiceUri(CONFIG_GRAPH_NAMES.platformImplicitMeshRootNodeConfig);
+ const expandedPlatformNodeDefaults = expandRelativeIds(PLATFORM_NODE_DEFAULTS, uri);
+ await createNewGraphFromJsonLd(expandedPlatformNodeDefaults, { graphName: uri });
+ } catch (error) {
+ throw new Error(`Failed to load platform mesh root node config: ${error instanceof Error ? error.message : String(error)}`);
+ }
}📝 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.
| export async function loadPlatformImplicitMeshRootNodeConfig(): Promise<void> { | |
| const uri = getCurrentServiceUri(CONFIG_GRAPH_NAMES.platformImplicitMeshRootNodeConfig); | |
| const expandedPlatformNodeDefaults = expandRelativeIds(PLATFORM_NODE_DEFAULTS, uri); | |
| await createNewGraphFromJsonLd(expandedPlatformNodeDefaults, { graphName: uri }); | |
| } | |
| export async function loadPlatformImplicitMeshRootNodeConfig(): Promise<void> { | |
| try { | |
| const uri = getCurrentServiceUri(CONFIG_GRAPH_NAMES.platformImplicitMeshRootNodeConfig); | |
| const expandedPlatformNodeDefaults = expandRelativeIds(PLATFORM_NODE_DEFAULTS, uri); | |
| await createNewGraphFromJsonLd(expandedPlatformNodeDefaults, { graphName: uri }); | |
| } catch (error) { | |
| throw new Error(`Failed to load platform mesh root node config: ${error instanceof Error ? error.message : String(error)}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In flow-service/src/config/loaders/quadstore-loader.ts around lines 19 to 23,
the function loadPlatformImplicitMeshRootNodeConfig lacks error handling. Wrap
the existing code in a try-catch block to catch any errors during the loading
process, and log or handle the error appropriately to prevent unhandled
exceptions and improve reliability.
| export async function loadInputServiceConfig(inputConfig: ServiceConfigInput): Promise<void> { | ||
| //console.log(inputConfig); | ||
| const uri = getCurrentServiceUri(CONFIG_GRAPH_NAMES.inputServiceConfig); | ||
| const expandedInputServiceConfig = expandRelativeIds(inputConfig, uri); | ||
| //console.log(expandedInputServiceConfig) | ||
| await createNewGraphFromJsonLd(expandedInputServiceConfig, { graphName: uri }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented code and add error handling.
The function contains commented debugging code that should be removed, and lacks error handling.
export async function loadInputServiceConfig(inputConfig: ServiceConfigInput): Promise<void> {
- //console.log(inputConfig);
- const uri = getCurrentServiceUri(CONFIG_GRAPH_NAMES.inputServiceConfig);
- const expandedInputServiceConfig = expandRelativeIds(inputConfig, uri);
- //console.log(expandedInputServiceConfig)
- await createNewGraphFromJsonLd(expandedInputServiceConfig, { graphName: uri });
+ try {
+ const uri = getCurrentServiceUri(CONFIG_GRAPH_NAMES.inputServiceConfig);
+ const expandedInputServiceConfig = expandRelativeIds(inputConfig, uri);
+ await createNewGraphFromJsonLd(expandedInputServiceConfig, { graphName: uri });
+ } catch (error) {
+ throw new Error(`Failed to load input service config: ${error instanceof Error ? error.message : String(error)}`);
+ }
}📝 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.
| export async function loadInputServiceConfig(inputConfig: ServiceConfigInput): Promise<void> { | |
| //console.log(inputConfig); | |
| const uri = getCurrentServiceUri(CONFIG_GRAPH_NAMES.inputServiceConfig); | |
| const expandedInputServiceConfig = expandRelativeIds(inputConfig, uri); | |
| //console.log(expandedInputServiceConfig) | |
| await createNewGraphFromJsonLd(expandedInputServiceConfig, { graphName: uri }); | |
| } | |
| export async function loadInputServiceConfig(inputConfig: ServiceConfigInput): Promise<void> { | |
| try { | |
| const uri = getCurrentServiceUri(CONFIG_GRAPH_NAMES.inputServiceConfig); | |
| const expandedInputServiceConfig = expandRelativeIds(inputConfig, uri); | |
| await createNewGraphFromJsonLd(expandedInputServiceConfig, { graphName: uri }); | |
| } catch (error) { | |
| throw new Error(`Failed to load input service config: ${error instanceof Error ? error.message : String(error)}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In flow-service/src/config/loaders/quadstore-loader.ts around lines 28 to 34,
remove the commented out console.log debugging statements and add proper error
handling to the async function loadInputServiceConfig. Wrap the existing code in
a try-catch block and handle any errors by logging or rethrowing them
appropriately to ensure failures are managed gracefully.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
flow-service/src/config/resolution/service-config-resolver.ts (3)
4-4: Import organization: Consider grouping related imports.The import for
ServiceOptionsis separated from other type imports. Consider grouping it with the type imports on lines 1-4 for better organization.import type { ServiceConfigInput, - ServiceOptions, + ServiceOptions, } from '../config-types.ts';
23-23: Import placement: Move imports to the top of the file.The import statement is placed after the function JSDoc comment, which is unconventional. Consider moving all imports to the top of the file for better readability and consistency.
import { createServiceLogContext } from '../../utils/service-log-context.ts'; import { serviceUriConfigManager, type ServiceUriConfig } from '../../utils/service-uri-builder.ts'; +import { loadPlatformServiceDefaults, loadInputServiceConfig, loadInputMeshRootNodeConfig, mergeServiceConfigGraphs } from '../loaders/quadstore-loader.ts'; /** * Asynchronously resolves the service configuration by merging CLI options, environment variables, configuration files, and environment-specific defaults in a defined precedence order. * * @param cliOptions - Optional CLI-provided service options to override other configuration sources * @throws ConfigError if configuration resolution fails or an unexpected error occurs */ -import { loadPlatformServiceDefaults, loadInputServiceConfig, loadInputMeshRootNodeConfig, mergeServiceConfigGraphs } from '../loaders/quadstore-loader.ts';
64-65: Address the TODO comment for mesh node config loading.The TODO comment indicates incomplete implementation for loading input mesh root node config. This might be intentional for this PR, but should be tracked.
The TODO suggests that mesh root node config loading is not yet implemented. Would you like me to create a tracking issue for this feature or help implement the mesh node config loading logic?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
flow-service/src/config/index.ts(2 hunks)flow-service/src/config/loaders/quadstore-loader.ts(1 hunks)flow-service/src/config/resolution/service-config-resolver.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- flow-service/src/config/loaders/quadstore-loader.ts
🔇 Additional comments (10)
flow-service/src/config/resolution/service-config-resolver.ts (4)
49-56: Early service URI config extraction and initialization looks correct.The approach of extracting service URI configuration early and initializing the service URI config manager before Quadstore operations is well-designed. This ensures the service URI is available for any subsequent operations that might need it.
71-78: Enhanced error context creation is well-implemented.The structured error context with operation, component, and config metadata provides excellent diagnostic information. This is a significant improvement over basic error handling.
108-110: New scheme support in CLI options conversion.The addition of scheme support in the CLI options conversion aligns with the service URI configuration requirements. The implementation correctly handles the optional scheme parameter.
25-27: All callers updated to handle void returnThe
resolveServiceConfigfunction now returnsvoid, and no callers expect a return value:• flow-service/src/config/index.ts
• Line 25:await resolveServiceConfig(cliOptions);
• No assignments or usages of a returned value foundNo further changes are needed.
flow-service/src/config/index.ts (6)
13-19: CONFIG_GRAPH_NAMES constant provides clear graph identification.The named graph constants provide clear identification for different configuration graphs in Quadstore. This is a good practice for maintainability and debugging.
23-36: Type export reorganization improves clarity.The updated type exports with more specific names like
MeshRootNodeConfigandFlowServiceContextprovide better semantic clarity than the previous generic names. This improves code readability and maintainability.
97-102: Enhanced error context in createServiceConfig is comprehensive.The structured error context with operation, component, and metadata provides excellent diagnostic information. The inclusion of CLI options in the metadata will help with debugging configuration issues.
90-92: No breaking changes in createServiceConfig usagesI’ve verified all
createServiceConfig(calls across the codebase and confirmed they simplyawait createServiceConfig();without expecting a return value. The new singleton accessor pattern is in place and no callers assign or use its return. No further action required.
64-64: No internal callers of validateJsonLd found
I searched the entire repo for invocations of validateJsonLd and only found its definition and re-export—no internal call sites to update. The simplified signature won’t break anything here.If this function is used by downstream consumers, please verify those external callers match the new one-arg signature.
72-74: Concurrency safety confirmed for singletonServiceConfigAccessorThe
singletonServiceConfigAccessoronly provides read-only getters against the shared Quadstore bundle (initialized once at startup). It does not mutate any state and relies on the SPARQL engine—which supports handling multiple simultaneous queries—for all its operations. You can safely ignore the original concurrency concern.Likely an incorrect or invalid review comment.
|
1 participant
@CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Style
Tests
Chores