Update/folder structure#2
Conversation
WalkthroughIntroduces a new Express-based API Gateway with trade creation, environment-configured ClickHouse initialization, and graceful shutdown. Adds a ClickHouse client package with a repository for trades. Establishes infrastructure via Docker (ClickHouse, Kafka, Redis, Postgres, Zookeeper) and SQL schema for trades and OHLCV views. Updates docs/tooling, adds types, and removes legacy API remnants. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as API Gateway (Express)
participant Ctrl as TradeController
participant Svc as TradeService
participant Repo as TradeRepository
participant CH as ClickHouse
Client->>API: POST /api/v1/trade { Trade }
API->>Ctrl: createTrade(req)
Ctrl->>Svc: createTrade(trade)
Svc->>Repo: create(trade)
Repo->>CH: INSERT INTO nerve.trade_data
CH-->>Repo: id
Repo-->>Svc: id
Svc-->>Ctrl: id
Ctrl-->>API: 201 { message }
API-->>Client: 201 Created
alt Insert error
Repo--x CH: INSERT
CH-->>Repo: error
Repo-->>Svc: throw
Svc-->>Ctrl: throw
Ctrl-->>API: next(error)
API-->>Client: 5xx { error payload }
end
sequenceDiagram
autonumber
participant SRV as Server bootstrap
participant Env as Env (dotenv/envalid)
participant Mgr as ClickHouseManager
participant CH as ClickHouse
SRV->>Env: Read CLICKHOUSE_* and PORT
SRV->>Mgr: initialize(config)
Mgr->>CH: createClient + ping
CH-->>Mgr: pong
Mgr-->>SRV: client ready
SRV->>SRV: start Express listen(PORT)
alt Shutdown (SIGINT/SIGTERM)
SRV->>Mgr: close()
Mgr->>CH: close connection
CH-->>Mgr: closed
Mgr-->>SRV: done
SRV->>SRV: process.exit(0)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (24)
lint-staged.config.js (1)
37-38: Use consistent equality operators.The condition mixes
==(loose equality) and===(strict equality) operators. For consistency and to avoid potential type coercion issues, use===throughout.Apply this diff:
- if (match[1] == "eslint-config" || match[1] == "typescript-config" || match[1] === "types") + if (match[1] === "eslint-config" || match[1] === "typescript-config" || match[1] === "types") return acc;infra/docker/.env.example (1)
1-26: Add a trailing newline.The file is missing a trailing newline, which can cause issues with some Unix tools and Git diffs.
Add a blank line at the end of the file:
# Kafka KAFKA_PORT=9092 KAFKA_EXTERNAL_PORT=9093 +Note: The dotenv-linter warnings about key ordering are stylistic preferences and don't affect functionality. The current grouping by service is more readable.
README.md (1)
167-167: Use a proper heading instead of emphasis.The "End of Startup Guide" line uses bold emphasis instead of a heading, which affects document structure and navigation.
Apply this diff:
-**End of Startup Guide** +## End of Startup Guidepackages/types/package.json (1)
13-15: Fill in license and author metadata.The
licenseandauthorfields are empty. For proper attribution and compliance, please populate these fields appropriately. If this is an internal package, consider using "UNLICENSED" for the license field.docs/index.md (2)
35-59: Add language identifier to fenced code block.The fenced code block lacks a language identifier. Adding one improves rendering and syntax highlighting.
Apply this diff:
-``` +```text exchange-platform/
169-169: Use heading syntax instead of emphasis.The "End of Documentation" marker uses emphasis (
**bold**) instead of proper heading syntax, which affects document structure and navigation.Apply this diff:
-**End of Documentation** +## End of Documentationapps/api-gateway/src/config/dotenv.ts (1)
12-15: LGTM! Consider secrets management for production.The ClickHouse configuration is correctly implemented with required environment variables and no defaults for sensitive credentials. For production deployments, consider using a secrets management solution (e.g., AWS Secrets Manager, HashiCorp Vault) instead of plain environment variables for
CLICKHOUSE_PASSWORD.package.json (2)
36-38: Document required arguments for Kafka scripts.The
kafka:topics:create,kafka:consumer, andkafka:producerscripts require topic name arguments but this isn't documented. Consider adding usage examples to the README or inline comments.Example usage should be documented:
# Create topic pnpm kafka:topics:create my-topic # Consume from topic pnpm kafka:consumer my-topic # Produce to topic pnpm kafka:producer my-topic
29-38: Verify cross-platform compatibility of shell scripts.These scripts use bash-specific syntax (
docker ps -qf, command substitution, pipes) that may not work on Windows without WSL. Additionally, thedocker ps -qf name=<service>pattern assumes unique container naming and could fail if multiple matching containers exist.Consider:
- Adding platform-specific scripts or documenting the requirement for bash/WSL on Windows
- Using more robust container identification (e.g., docker compose ps -q )
Would you like me to generate cross-platform compatible versions of these scripts?
packages/clickhouse/src/client.ts (1)
33-37: Review connection pool and keep-alive settings.The current settings may need adjustment for production:
max_open_connections: 10might be low for high-load scenarioskeep_alive.idle_socket_ttl: 2500(2.5s) is quite short and may cause frequent reconnectionsConsider making these configurable via the
IClickHouseConfiginterface or environment variables to allow tuning per deployment environment.Based on learnings from @clickhouse/client 1.12.1 documentation.
packages/logger/package.json (1)
23-23: Remove unused tsup dependency.
tsupis listed indevDependenciesbut is no longer used in the build script. This can be removed to keep dependencies clean.Apply this diff:
"@repo/eslint-config": "workspace:*", "@repo/typescript-config": "workspace:*", - "tsup": "^8.5.0", "typescript": "^5.9.2"apps/api-gateway/.env.example (1)
7-9: Document Docker Compose environment variables.
COMPOSE_PROJECT_NAME, CLICKHOUSE_HTTP_PORT, and CLICKHOUSE_NATIVE_PORT are used in infra/docker/docker-compose.yml for container names and port mappings—add comments or group them under a “Docker Compose” section in apps/api-gateway/.env.example.packages/types/trade.d.ts (1)
4-5: Consider documenting or strengthening timestamp formats.The
clientTimestampandengineTimestampfields are typed asstring. If these follow a specific format (e.g., ISO 8601), consider documenting this in JSDoc comments or using a branded type to ensure format consistency across the codebase.Example with JSDoc:
export interface Trade { + /** ISO 8601 timestamp string (e.g., "2025-10-04T13:12:49Z") */ clientTimestamp: string; + /** ISO 8601 timestamp string (e.g., "2025-10-04T13:12:49Z") */ engineTimestamp: string; symbol: string;apps/api-gateway/src/routers/trade.route.ts (1)
16-18: Avoid the double type cast with proper handler typing.The
as unknown as CreateTradeRequestcast indicates a type mismatch. SinceCreateTradeRequestextendsRequest, you can directly type the route handler parameters.Apply this diff to remove the cast:
-router.route("/").post((req: Request, res: Response, next: NextFunction) => { - tradeController.createTrade(req as unknown as CreateTradeRequest, res, next); -}); +router.route("/").post((req: CreateTradeRequest, res: Response, next: NextFunction) => { + tradeController.createTrade(req, res, next); +});apps/api-gateway/src/app.ts (1)
22-29: Consider populating or removing empty error fields.The error response includes
pathandlocationfields that are always empty strings. Either populate these with meaningful values (e.g.,req.path, error stack location) or remove them to avoid misleading API consumers.Option 1 (populate with request path):
{ type: err.name, msg: err.message, - path: "", + path: _req.path, location: "", },Option 2 (remove empty fields):
{ type: err.name, msg: err.message, - path: "", - location: "", },cickhouseInitialData.txt (1)
1-2: Consider making this an executable shell script.These curl commands would be more convenient as an executable
.shscript with appropriate shebang and error handling. This would allow:
- Direct execution:
./seed-clickhouse-data.sh- Better error handling and feedback
- Easy integration into setup/CI workflows
Example structure:
#!/bin/bash set -e CLICKHOUSE_HOST="${CLICKHOUSE_HOST:-http://localhost:8123}" echo "Seeding ClickHouse with initial trade data..." curl -X POST "$CLICKHOUSE_HOST/" \ --data-binary @- << 'EOF' ... EOF echo "Seeding complete!"infra/docker/docker-compose.yml (2)
1-1: Consider upgrading to Compose v3.9 or newer.Docker Compose v3.8 is older. Consider using v3.9 (released 2021) or the newer Compose Specification format (no version key) for better feature support and forward compatibility.
58-58: Remove commented-out code or document why it's preserved.The postgres init-scripts volume mount is commented out. Either:
- Remove it if not needed
- Uncomment it if needed
- Document why it's kept as a placeholder (e.g., "TODO: add postgres init scripts")
apps/api-gateway/src/services/trade.service.ts (1)
9-27: Consider simplifying by passing the entire Trade object.You're destructuring all properties from the Trade parameter and then immediately reconstructing the same object to pass to
tradeRepo.create(). This is redundant.Apply this diff:
- async createTrade({ - engineTimestamp, - clientTimestamp, - symbol, - price, - volume, - side, - userId, - }: Trade) { - await this.tradeRepo.create({ - engineTimestamp, - clientTimestamp, - symbol, - price, - volume, - side, - userId, - }); + async createTrade(trade: Trade) { + await this.tradeRepo.create(trade); }packages/clickhouse/src/repositories/trade.ts (2)
40-44: Validate the parsed JSON structure before casting.The
JSON.parse(row.text) as Tradecast is unsafe. If the database schema changes or returns unexpected data, this will silently create invalid Trade objects.Consider adding runtime validation:
for await (const rows of resultSet.stream()) { for (const row of rows) { const data = JSON.parse(row.text); // Add validation here - e.g., using zod or a custom validator if (isValidTrade(data)) { return data as Trade; } throw new Error("Invalid trade data from database"); } }Alternatively, use a schema validation library like zod to ensure type safety at runtime.
33-47: Add error handling for database query failures.The
findByIdmethod doesn't handle potential query failures. If the ClickHouse connection is down or the query fails, the error will propagate unhandled.Consider wrapping the query in a try-catch with appropriate error handling:
async findById(id: string): Promise<Trade | null> { try { const resultSet = await this.client.query({ query: "SELECT * FROM nerve.trade_data WHERE id = {id:UUID} LIMIT 1", query_params: { id }, format: "JSONEachRow", }); for await (const rows of resultSet.stream()) { for (const row of rows) { return JSON.parse(row.text) as Trade; } } return null; } catch (error) { // Log error and rethrow with context throw new Error(`Failed to find trade by id ${id}: ${error}`); } }apps/api-gateway/src/controllers/trade.controller.ts (2)
12-39: Simplify by passing req.body directly to the service.You're destructuring all Trade properties from
req.body(line 14), reconstructing them into an object (lines 16-24), and then logging all of them again (lines 26-34). This is redundant and verbose.Apply this diff:
async createTrade(req: CreateTradeRequest, res: Response, next: NextFunction) { try { - const { engineTimestamp, clientTimestamp, symbol, price, volume, side, userId } = req.body; - - await this.TradeService.createTrade({ - engineTimestamp, - clientTimestamp, - symbol, - price, - volume, - side, - userId, - }); + await this.TradeService.createTrade(req.body); - this.logger.info("New trade is created", { - engineTimestamp, - clientTimestamp, - symbol, - price, - volume, - side, - userId, - }); + this.logger.info("New trade is created", req.body); res.status(201).json({ message: "New trade is created" }); } catch (error) { return next(error); } }
35-35: Consider returning the created trade ID in the response.The response only returns a generic success message. It would be more useful to return the generated trade ID so clients can reference it.
Modify the service to return the trade ID, then include it in the response:
const tradeId = await this.TradeService.createTrade(req.body); this.logger.info("New trade is created", { tradeId, ...req.body }); res.status(201).json({ message: "New trade is created", tradeId });Note: This requires updating
TradeService.createTrade()to return the ID fromtradeRepo.create().infra/docker/clickhouse/init-scripts/01-init.sql (1)
247-266: Consider uncommenting the testing queries as a separate verification script.The commented testing queries are valuable for validation. Rather than keeping them commented in the init script, consider creating a separate
02-verify.sqlscript that can be run independently to verify the setup.Create
infra/docker/clickhouse/init-scripts/02-verify.sql:-- Verify no duplicates in candles SELECT 'Checking for duplicate 1m candles' as check_name, count(*) as duplicates FROM ( SELECT symbol, candle_time, count(*) as cnt FROM nerve.candles_1m GROUP BY symbol, candle_time HAVING cnt > 1 ); -- Check record counts across all tables SELECT 'Raw trades' as table_name, count() as record_count FROM nerve.trade_data UNION ALL SELECT '1m candles', count() FROM nerve.candles_1m UNION ALL SELECT '5m candles', count() FROM nerve.candles_5m UNION ALL SELECT '4h candles', count() FROM nerve.candles_4h UNION ALL SELECT '1d candles', count() FROM nerve.candles_1d;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
apps/api/logs/combined-2025-09-13.logis excluded by!**/*.logapps/api/logs/error-2025-09-13.logis excluded by!**/*.logapps/api/logs/exceptions-2025-09-13.logis excluded by!**/*.logapps/api/logs/rejections-2025-09-13.logis excluded by!**/*.logapps/web/public/vite.svgis excluded by!**/*.svgapps/web/src/assets/react.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
.gitignore(1 hunks)README.md(1 hunks)apps/api-gateway/.env.example(1 hunks)apps/api-gateway/package.json(1 hunks)apps/api-gateway/src/app.ts(1 hunks)apps/api-gateway/src/config/dotenv.ts(1 hunks)apps/api-gateway/src/controllers/trade.controller.ts(1 hunks)apps/api-gateway/src/routers/trade.route.ts(1 hunks)apps/api-gateway/src/server.ts(1 hunks)apps/api-gateway/src/services/trade.service.ts(1 hunks)apps/api-gateway/src/types/index.ts(1 hunks)apps/api-gateway/tsconfig.json(1 hunks)apps/api/.env.example(0 hunks)apps/api/logs/.2abc85c82cdda96487f9b2edd172659f093a115b-audit.json(0 hunks)apps/api/logs/.52496fbede3b4e1e307973b1f7482eea6a9e1553-audit.json(0 hunks)apps/api/logs/.6a8385029aaed3383ca37d3aa171a144e160f8bf-audit.json(0 hunks)apps/api/logs/.e5210eccd550f9edac74ec60024b73718af4e4e8-audit.json(0 hunks)apps/api/src/server.ts(0 hunks)cickhouseInitialData.txt(1 hunks)docs/index.md(1 hunks)infra/docker/.env.example(1 hunks)infra/docker/clickhouse/init-scripts/01-init.sql(1 hunks)infra/docker/docker-compose.yml(1 hunks)lint-staged.config.js(1 hunks)package.json(1 hunks)packages/clickhouse/eslint.config.mjs(1 hunks)packages/clickhouse/package.json(1 hunks)packages/clickhouse/src/client.ts(1 hunks)packages/clickhouse/src/index.ts(1 hunks)packages/clickhouse/src/repositories/trade.ts(1 hunks)packages/clickhouse/tsconfig.json(1 hunks)packages/logger/package.json(1 hunks)packages/logger/tsconfig.json(1 hunks)packages/types/index.d.ts(1 hunks)packages/types/package.json(1 hunks)packages/types/trade.d.ts(1 hunks)packages/types/tsconfig.json(1 hunks)packages/typescript-config/node.json(1 hunks)packages/ui/src/button.tsx(1 hunks)turbo.json(2 hunks)
💤 Files with no reviewable changes (6)
- apps/api/src/server.ts
- apps/api/logs/.6a8385029aaed3383ca37d3aa171a144e160f8bf-audit.json
- apps/api/logs/.e5210eccd550f9edac74ec60024b73718af4e4e8-audit.json
- apps/api/logs/.52496fbede3b4e1e307973b1f7482eea6a9e1553-audit.json
- apps/api/logs/.2abc85c82cdda96487f9b2edd172659f093a115b-audit.json
- apps/api/.env.example
🧰 Additional context used
🧬 Code graph analysis (8)
apps/api-gateway/src/routers/trade.route.ts (5)
packages/clickhouse/src/repositories/trade.ts (1)
TradeRepository(5-48)apps/api-gateway/src/services/trade.service.ts (1)
TradeService(6-32)apps/api-gateway/src/controllers/trade.controller.ts (1)
TradeController(6-40)packages/logger/src/index.ts (1)
logger(109-109)apps/api-gateway/src/types/index.ts (1)
CreateTradeRequest(4-6)
packages/clickhouse/src/repositories/trade.ts (3)
packages/clickhouse/src/index.ts (2)
TradeRepository(2-2)clickHouseManager(1-1)packages/clickhouse/src/client.ts (1)
clickHouseManager(58-58)packages/types/trade.d.ts (1)
Trade(3-11)
apps/api-gateway/src/app.ts (1)
packages/logger/src/index.ts (1)
logger(109-109)
apps/api-gateway/src/services/trade.service.ts (2)
packages/clickhouse/src/repositories/trade.ts (1)
TradeRepository(5-48)packages/types/trade.d.ts (1)
Trade(3-11)
packages/clickhouse/src/client.ts (1)
packages/clickhouse/src/index.ts (2)
IClickHouseConfig(1-1)clickHouseManager(1-1)
apps/api-gateway/src/types/index.ts (1)
packages/types/trade.d.ts (1)
Trade(3-11)
apps/api-gateway/src/controllers/trade.controller.ts (3)
apps/api-gateway/src/services/trade.service.ts (1)
TradeService(6-32)packages/logger/src/index.ts (2)
logger(109-109)Logger(115-115)apps/api-gateway/src/types/index.ts (1)
CreateTradeRequest(4-6)
apps/api-gateway/src/server.ts (3)
packages/clickhouse/src/client.ts (1)
clickHouseManager(58-58)apps/api-gateway/src/config/dotenv.ts (1)
env(9-16)packages/logger/src/index.ts (1)
logger(109-109)
🪛 dotenv-linter (3.3.0)
infra/docker/.env.example
[warning] 6-6: [UnorderedKey] The CLICKHOUSE_HTTP_PORT key should go before the CLICKHOUSE_URL key
(UnorderedKey)
[warning] 7-7: [UnorderedKey] The CLICKHOUSE_NATIVE_PORT key should go before the CLICKHOUSE_URL key
(UnorderedKey)
[warning] 8-8: [UnorderedKey] The CLICKHOUSE_DB key should go before the CLICKHOUSE_HTTP_PORT key
(UnorderedKey)
[warning] 10-10: [UnorderedKey] The CLICKHOUSE_PASSWORD key should go before the CLICKHOUSE_URL key
(UnorderedKey)
[warning] 17-17: [UnorderedKey] The POSTGRES_DB key should go before the POSTGRES_PORT key
(UnorderedKey)
[warning] 19-19: [UnorderedKey] The POSTGRES_PASSWORD key should go before the POSTGRES_PORT key
(UnorderedKey)
[warning] 26-26: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 26-26: [UnorderedKey] The KAFKA_EXTERNAL_PORT key should go before the KAFKA_PORT key
(UnorderedKey)
apps/api-gateway/.env.example
[warning] 2-2: [UnorderedKey] The NODE_ENV key should go before the PORT key
(UnorderedKey)
[warning] 3-3: [UnorderedKey] The CLICKHOUSE_DB key should go before the NODE_ENV key
(UnorderedKey)
[warning] 4-4: [UnorderedKey] The CLICKHOUSE_USER key should go before the NODE_ENV key
(UnorderedKey)
[warning] 5-5: [UnorderedKey] The CLICKHOUSE_PASSWORD key should go before the CLICKHOUSE_USER key
(UnorderedKey)
[warning] 6-6: [UnorderedKey] The CLICKHOUSE_URL key should go before the CLICKHOUSE_USER key
(UnorderedKey)
[warning] 7-7: [UnorderedKey] The COMPOSE_PROJECT_NAME key should go before the NODE_ENV key
(UnorderedKey)
[warning] 8-8: [UnorderedKey] The CLICKHOUSE_HTTP_PORT key should go before the CLICKHOUSE_PASSWORD key
(UnorderedKey)
[warning] 9-9: [UnorderedKey] The CLICKHOUSE_NATIVE_PORT key should go before the CLICKHOUSE_PASSWORD key
(UnorderedKey)
🪛 markdownlint-cli2 (0.18.1)
README.md
167-167: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
docs/index.md
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
169-169: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (27)
.gitignore (1)
42-43: Good call on ignoring build artifacts.Adding
tsconfig.tsbuildinfoand generallogskeeps transient build output and runtime logs out of version control. 👍packages/ui/src/button.tsx (1)
13-13: LGTM!The formatting change consolidates the button tag to a single line with no behavioral impact.
apps/api-gateway/tsconfig.json (1)
5-5: LGTM!The addition of
rootDir: "src"aligns the API gateway configuration with other packages in the monorepo and ensures consistent build behavior.packages/clickhouse/eslint.config.mjs (1)
1-3: LGTM!The ESLint configuration correctly re-exports the shared Node.js config, maintaining consistency across the monorepo.
turbo.json (4)
11-12: LGTM!Adding
dependsOn: ["^build"]ensures that dependencies are built before starting development servers, preventing runtime errors from missing artifacts. Thepersistent: truesetting is appropriate for long-running dev servers.
20-23: LGTM!Adding
dependsOn: ["^build"]ensures type checking runs against built dependencies, andoutputs: []correctly indicates that type checking doesn't produce cacheable artifacts.
24-30: LGTM!The new
cleanandinfra:uptasks are correctly configured:
cleanwithcache: falseprevents caching of deletion operationsinfra:upwith empty outputs indicates infrastructure startup doesn't produce build artifacts
5-7: Consider retaining.env*in Turbo build inputs
Our search ofpackage.jsonscripts and common build configs (Vite/Next/Webpack) found no directprocess.envor dotenv usage, but absence of matches isn’t proof. Manually confirm your build doesn’t bake in environment variables; if it does, restore.env*toturbo.jsoninputs or document that env-changes require a manual rebuild.infra/docker/.env.example (1)
10-19: LGTM! Verify passwords are for development only.The default passwords ("nerve") are appropriate for local development. Ensure these are never used in production environments and that the documentation clearly indicates this is a development-only configuration.
Consider adding a comment to clarify this is for development:
# ClickHouse CLICKHOUSE_URL=http://localhost:8123 CLICKHOUSE_HTTP_PORT=8123 CLICKHOUSE_NATIVE_PORT=9000 CLICKHOUSE_DB=default CLICKHOUSE_USER=default +# Default password for local development only - DO NOT use in production CLICKHOUSE_PASSWORD=nerveREADME.md (1)
1-166: LGTM! Comprehensive startup documentation.The updated README provides clear, step-by-step instructions for:
- Installing dependencies
- Starting infrastructure services
- Health checking
- Running applications
- Managing Kafka topics
The PNPM-based workflow is well-documented and aligns with the monorepo structure changes in this PR.
apps/api-gateway/package.json (1)
24-24: Verify Express v5 compatibility.The package uses Express ^5.1.0 (latest). Express v5 introduced breaking changes from v4, including route matching behavior and middleware compatibility. Ensure that:
- All route definitions have been updated per the v5 migration guide
- Third-party middleware is compatible with Express v5
- The official codemod has been run if migrating from v4
Based on learnings about Express v5.1.0.
packages/logger/package.json (1)
15-15: Ensure build outputs match exports & remove unused tsup.The
tsc -p tsconfig.jsonbuild must generatedist/index.js,dist/index.mjs, anddist/index.d.tsas declared inexports. Verify locally that all three files exist; if any are missing, removetsupfrom devDependencies, updateexportsto the actual outputs, or adjust your tsc configuration to emit both CJS and ESM.packages/types/index.d.ts (1)
1-1: LGTM!The re-export pattern is standard and appropriate for a types package index.
packages/clickhouse/tsconfig.json (1)
1-6: LGTM!The TypeScript configuration follows standard conventions and properly configures the build output structure.
packages/clickhouse/src/index.ts (1)
1-2: LGTM!The public API surface is well-defined with appropriate re-exports for the ClickHouse package.
apps/api-gateway/src/types/index.ts (1)
1-6: LGTM!The CreateTradeRequest type properly extends Express Request with Trade-specific body typing.
apps/api-gateway/src/server.ts (2)
6-19: LGTM!The database initialization logic properly handles errors and exits on failure, preventing the server from running without a database connection.
22-32: LGTM!The graceful shutdown handlers for both SIGINT and SIGTERM properly close the ClickHouse connection before exiting.
apps/api-gateway/src/app.ts (1)
1-13: LGTM!The Express app setup with JSON parsing and route mounting follows best practices.
infra/docker/docker-compose.yml (2)
24-27: Document the rationale for the high file descriptor limit.The ulimit of 262,144 is exceptionally high (default is typically 1,024). While ClickHouse can benefit from higher limits for concurrent queries and large datasets, this specific value should be documented to explain why it's necessary for your workload.
Consider adding a comment in the docker-compose file or in documentation explaining:
- Expected query concurrency
- Dataset size projections
- Performance benchmarks that led to this configuration
89-91: LGTM! Proper healthcheck dependency.The use of
condition: service_healthyensures Kafka only starts after Zookeeper is ready, preventing connection failures during startup.packages/typescript-config/node.json (1)
4-7: LGTM! Modern ES2022 with Node16 module resolution.The configuration correctly targets ES2022 with Node16 module resolution, which provides proper ESM/CJS interop for Node.js projects. This is a good modernization step.
Based on learnings: Node16 module resolution is the recommended approach for Node.js TypeScript projects targeting modern Node versions.
packages/clickhouse/package.json (1)
1-33: LGTM! Package structure is well-configured.The package manifest correctly defines:
- Dual export paths (types and default)
- Standard build/lint/clean scripts
- Appropriate dependencies including the latest stable @clickhouse/client@^1.12.1
Based on learnings: @clickhouse/client 1.12.1 is the latest stable version with performance improvements and Node.js 24.x support.
infra/docker/clickhouse/init-scripts/01-init.sql (4)
10-27: LGTM! Well-designed trade data table with appropriate indexes.The trade_data table schema is well-designed:
- UUID for unique identification
- DateTime64(3) for millisecond precision timestamps
- LowCardinality for symbol (efficient for repetitive string values)
- Float64 for financial precision
- Enum for side (space-efficient)
- Proper partitioning by day
- Strategic indexes on price, user_id, and id
36-93: LGTM! Proper use of AggregatingMergeTree for incremental aggregation.The state tables correctly use
AggregatingMergeTreewith appropriate aggregate functions:
argMin/argMaxfor open/close with timestamp orderingmax/minfor high/lowsumfor volume and volume-weighted calculationscountfor trade countThe partitioning strategies are sensible:
- Monthly partitioning for 1m, 5m candles
- Daily partitioning for 4h candles
- Monthly partitioning for 1d candles with Date type
101-162: Verify the time bucketing logic for 5m and 4h candles.The time bucketing uses
intDiv(toUnixTimestamp(...), interval) * interval:
- 5-minute:
intDiv(toUnixTimestamp(engine_timestamp), 300) * 300(Line 121)- 4-hour:
intDiv(toUnixTimestamp(engine_timestamp), 14400) * 14400(Line 137)Please verify this logic handles timezone and DST transitions correctly. ClickHouse's
toStartOfFiveMinutes()andtoStartOfInterval()functions are purpose-built for this and may be more robust.Consider this alternative:
-- Instead of: toDateTime(intDiv(toUnixTimestamp(engine_timestamp), 300) * 300) AS candle_time -- Use: toStartOfInterval(engine_timestamp, INTERVAL 5 MINUTE) AS candle_timeDo you want me to generate a script to verify the time bucketing produces expected results?
182-184: LGTM! Safe handling of zero-volume edge case in VWAP calculation.The
if(sumMerge(volume_state) > 0, ..., 0)pattern correctly prevents division by zero when calculating VWAP, returning 0 as a fallback. This is consistent across all candle views.
| "type": "commonjs", | ||
| "scripts": { | ||
| "dev": "turbo build --filter=@repo/logger && nodemon", | ||
| "dev": "nodemon", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify logger build dependency is handled.
The dev script no longer includes the turbo build --filter=@repo/logger step. Ensure that the logger package is built before running dev, either through workspace dependency resolution or a separate build step.
Run this script to verify the dev workflow:
🏁 Script executed:
#!/bin/bash
# Description: Verify that logger is built when running dev script
# Check if turbo handles workspace dependencies automatically
cd apps/api-gateway
pnpm dev &
DEV_PID=$!
sleep 2
kill $DEV_PID 2>/dev/null
# Check if logger dist exists
if [ -d "../../packages/logger/dist" ]; then
echo "✓ Logger package is built"
else
echo "✗ Logger package is not built - dev script may fail"
exit 1
fiLength of output: 270
Ensure logger is built before running dev
The dev script in apps/api-gateway/package.json only runs nodemon and no longer includes turbo build --filter=@repo/logger, so packages/logger/dist is missing and pnpm dev fails. Add a pre-build step (e.g. turbo run build --filter=@repo/logger) or configure workspace dependencies to build @repo/logger before dev.
🤖 Prompt for AI Agents
In apps/api-gateway/package.json around line 8, the "dev" script only runs
"nodemon" which fails because packages/logger/dist isn't built; update the
script to build the logger first (for example prepend a pre-step like running
"turbo run build --filter=@repo/logger" or "turbo build --filter=@repo/logger")
so @repo/logger is compiled before nodemon runs, or alternatively configure
workspace/devDependencies so the logger package is built as a workspace
dependency prior to running dev.
| "noUnusedLocals": true, | ||
| "noUnusedParameters": true, | ||
| "exactOptionalPropertyTypes": true, | ||
| "verbatimModuleSyntax": false, | ||
| "noUncheckedSideEffectImports": true, | ||
| "allowImportingTsExtensions": false | ||
| "noImplicitReturns": true, | ||
| "noFallthroughCasesInSwitch": true, | ||
| "noUncheckedIndexedAccess": true |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that new strict checks don't break existing code.
The addition of noUnusedLocals, noUnusedParameters, noImplicitReturns, noFallthroughCasesInSwitch, and noUncheckedIndexedAccess will surface new type errors in existing code.
Run the following script to check for type errors across the workspace:
🏁 Script executed:
#!/bin/bash
# Description: Check for TypeScript errors in all packages that extend this config
echo "Checking for TypeScript errors after strict flag additions..."
# Find all packages that extend this config
fd -t f 'tsconfig.json' -x grep -l '@repo/typescript-config/node.json' {} \; | while read -r tsconfig; do
dir=$(dirname "$tsconfig")
echo "Checking $dir..."
(cd "$dir" && npm run type-check 2>&1) || echo "⚠️ Type errors found in $dir"
doneLength of output: 804
Ensure strict flags are validated in each package
Add typescript as a devDependency and a type-check script (e.g. "type-check": "tsc --noEmit -p tsconfig.json") to every package extending this config, then run npm run type-check (or npx tsc -p tsconfig.json) to catch and fix any type errors introduced by the new strict flags.
🤖 Prompt for AI Agents
In packages/typescript-config/node.json around lines 21 to 26, the strict
TypeScript flags were enabled but downstream packages that extend this config
may not have TypeScript installed or a type-check script; add "typescript" as a
devDependency in each package.json that extends this config and add a
"type-check" script (for example "tsc --noEmit -p tsconfig.json"); run npm run
type-check (or npx tsc -p tsconfig.json) in each package and fix any reported
type errors (unused locals/params, implicit returns, exact optional types,
unchecked indexed access, fallthrough cases) until the type-check passes.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/logger/package.json (1)
21-24: Drop unusedtsupdependency.With the build script now using
tsc -p tsconfig.json,tsupis no longer referenced anywhere. Removing it trims install time and keeps the manifest honest."devDependencies": { "@repo/eslint-config": "workspace:*", "@repo/typescript-config": "workspace:*", - "tsup": "^8.5.0", "typescript": "^5.9.2" },apps/api-gateway/src/server.ts (1)
43-48: Strengthen error handling to catch all error types.The catch block only handles
Errorinstances, but JavaScript allows throwing any value. Non-Error throws would silently fail without logging or process exit.Apply this diff:
} catch (error) { - if (error instanceof Error) { - logger.error(error.message); - process.exit(1); - } + logger.error("Failed to start server", error instanceof Error ? error.message : String(error)); + process.exit(1); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
apps/api-gateway/package.json(1 hunks)apps/api-gateway/src/app.ts(1 hunks)apps/api-gateway/src/server.ts(1 hunks)apps/api-gateway/src/services/trade.service.ts(1 hunks)clickhouseInitialData.txt(1 hunks)package.json(1 hunks)packages/clickhouse/package.json(1 hunks)packages/clickhouse/src/client.ts(1 hunks)packages/clickhouse/src/repositories/trade.ts(1 hunks)packages/logger/package.json(1 hunks)packages/logger/tsconfig.json(1 hunks)packages/types/tsconfig.json(1 hunks)packages/ui/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- clickhouseInitialData.txt
- packages/types/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/clickhouse/package.json
- packages/clickhouse/src/client.ts
- packages/clickhouse/src/repositories/trade.ts
- package.json
- apps/api-gateway/src/services/trade.service.ts
- apps/api-gateway/package.json
🧰 Additional context used
🧬 Code graph analysis (2)
apps/api-gateway/src/server.ts (3)
packages/clickhouse/src/client.ts (1)
clickHouseManager(65-65)apps/api-gateway/src/config/dotenv.ts (1)
env(9-16)packages/logger/src/index.ts (1)
logger(109-109)
apps/api-gateway/src/app.ts (1)
packages/logger/src/index.ts (1)
logger(109-109)
🔇 Additional comments (4)
apps/api-gateway/src/server.ts (2)
6-19: LGTM! Database initialization is properly structured.The initialization function correctly handles connection setup and provides clear error logging. The immediate exit on failure is appropriate for preventing the application from running without database connectivity.
51-51: LGTM! Non-blocking server start is appropriate.Using
void startServer()correctly indicates the intentionally floating promise for the top-level async invocation.apps/api-gateway/src/app.ts (2)
1-7: LGTM! Express setup follows best practices.The app initialization and JSON middleware configuration align with Express 5.x conventions.
Based on learnings
9-13: LGTM! Routes are properly mounted.The trade router is correctly mounted with versioned API prefix, and the root endpoint provides a functional health check.
| app.use((err: HttpError, _req: Request, res: Response, _next: NextFunction) => { | ||
| if (err instanceof Error) { | ||
| logger.error(err.message); | ||
| const statusCode = err.status || err.statusCode || 500; | ||
|
|
||
| res.status(statusCode).json({ | ||
| error: [ | ||
| { | ||
| type: err.name, | ||
| msg: err.message, | ||
| path: "", | ||
| location: "", | ||
| }, | ||
| ], | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Populate error context fields and simplify response structure.
The error handler has several improvement opportunities:
- Empty path and location: Lines 26-27 should populate actual request context
- Unnecessary array wrapping: Single errors don't need array wrapping
- Type narrowing: The
instanceof Errorcheck on line 17 is redundant given theHttpErrorparameter type
Apply this diff to improve the error handler:
// eslint-disable-next-line @typescript-eslint/no-unused-vars
app.use((err: HttpError, req: Request, res: Response, _next: NextFunction) => {
- if (err instanceof Error) {
- logger.error(err.message);
- const statusCode = err.status || err.statusCode || 500;
+ logger.error(err.message);
+ const statusCode = err.status || err.statusCode || 500;
- res.status(statusCode).json({
- error: [
- {
- type: err.name,
- msg: err.message,
- path: "",
- location: "",
- },
- ],
- });
- }
+ res.status(statusCode).json({
+ error: {
+ type: err.name,
+ msg: err.message,
+ path: req.path,
+ location: req.method,
+ },
+ });
});Committable suggestion skipped: line range outside the PR's diff.
| process.on("SIGINT", async () => { | ||
| logger.info("Shutting down server..."); | ||
| await clickHouseManager.close(); | ||
| process.exit(0); | ||
| }); | ||
|
|
||
| process.on("SIGTERM", async () => { | ||
| logger.info("Shutting down server..."); | ||
| await clickHouseManager.close(); | ||
| process.exit(0); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicate shutdown logic into a shared handler.
Both SIGINT and SIGTERM handlers contain identical shutdown code, violating DRY principles. Additionally, the handlers close only the ClickHouse connection but not the HTTP server itself, which could leave open connections.
Apply this diff to deduplicate and improve graceful shutdown:
+// Handle graceful shutdown
+async function gracefulShutdown(signal: string) {
+ logger.info(`Received ${signal}, shutting down gracefully...`);
+ await clickHouseManager.close();
+ // Close HTTP server if it exists
+ if (server) {
+ server.close(() => {
+ logger.info("HTTP server closed");
+ process.exit(0);
+ });
+ } else {
+ process.exit(0);
+ }
+}
+
-// Handle graceful shutdown
-process.on("SIGINT", async () => {
- logger.info("Shutting down server...");
- await clickHouseManager.close();
- process.exit(0);
-});
-
-process.on("SIGTERM", async () => {
- logger.info("Shutting down server...");
- await clickHouseManager.close();
- process.exit(0);
-});
+process.on("SIGINT", () => void gracefulShutdown("SIGINT"));
+process.on("SIGTERM", () => void gracefulShutdown("SIGTERM"));Then update startServer to store the server instance:
const startServer = async () => {
const PORT = env.PORT;
try {
await initializeDatabase();
- app.listen(PORT, () => {
+ server = app.listen(PORT, () => {
logger.info(`Server is running on port ${PORT}...`);
});
} catch (error) {And declare the server variable at module scope:
import app from "@/app";
+import type { Server } from "http";
+let server: Server | null = null;
+
async function initializeDatabase() {🤖 Prompt for AI Agents
In apps/api-gateway/src/server.ts around lines 22 to 32, the SIGINT and SIGTERM
handlers duplicate shutdown logic and only close ClickHouse; extract the shared
shutdown logic into a single async function (e.g., gracefulShutdown) that first
logs shutdown, closes the HTTP server instance (await server.close() or
promisified close) to drain connections, then closes clickHouseManager, and
finally exits the process; declare the server variable at module scope (let
server: http.Server | undefined) and update startServer to assign the returned
server instance to that module-scoped variable so the shared handler can
reference it.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores