Skip to content

COG-33: Extract reference data service from TraderX monorepo#7

Open
devin-ai-integration[bot] wants to merge 1 commit intomainfrom
devin/1771026623-extract-reference-data-service
Open

COG-33: Extract reference data service from TraderX monorepo#7
devin-ai-integration[bot] wants to merge 1 commit intomainfrom
devin/1771026623-extract-reference-data-service

Conversation

@devin-ai-integration
Copy link

@devin-ai-integration devin-ai-integration bot commented Feb 13, 2026

Summary

Extracts the reference-data NestJS service from COG-GTM/traderXCognitiondemos into this standalone repository. All source code, configuration, tests, data files, and Dockerfiles were copied verbatim from the reference-data/ directory of the monorepo.

What's included (27 files):

  • NestJS application (stocks controller/service, health check, CSV data loader)
  • S&P 500 reference data CSV (505 entries)
  • Unit tests (14 passing) and e2e test scaffold
  • Dockerfile, base.Dockerfile, ESLint/Prettier/TypeScript configs
  • OpenAPI spec (openapi.yaml)

Verified locally: npm install ✅ · npm run build ✅ · npm test (14/14 pass) ✅

Review & Testing Checklist for Human

  • base.Dockerfile references library-scripts/*.sh and library-scripts/*.env — these files do not exist in this repo. This Dockerfile will fail to build standalone. Decide whether to remove it or add the missing scripts.
  • Cross-service dependency documentation is missing from this PR. The ticket asks to "note where we need to update dependencies between services." Key consumers: trade-service (via REFERENCE_DATA_HOST env var), Angular frontend (refrenceDataUrl), React frontend (reference_data_url), docker-compose, k8s manifests, ingress nginx config, and Radius bicep template — all in the monorepo.
  • Dockerfile uses Node 16 devcontainer base image (mcr.microsoft.com/vscode/devcontainers/javascript-node:0-16-bullseye) which is EOL. Verify this is acceptable for a standalone service or if it should be updated.
  • test/app.e2e-spec.ts expects GET / → 200 "Hello World!" but the app has no root route handler — this e2e test will fail if run (npm run test:e2e). This is a pre-existing issue from the monorepo.
  • 4 pre-existing ESLint errors (unused imports in controller/spec, require statement in CSV loader). Identical to monorepo — confirm whether to address now or separately.
  • Run npm install && npm test locally to verify the extracted service builds and tests pass independently.

Notes


Open with Devin

Extract the NestJS reference-data service from COG-GTM/traderXCognitiondemos
into this standalone repository. Includes all source code, tests, data files,
configuration, and Docker files.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration
Copy link
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Author

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 2 potential issues.

View 6 additional findings in Devin Review.

Open in Devin Review

Comment on lines +6 to +20
const promise = new Promise<Stock[]>((resolve) => {
const stocks: Stock[] = [];
let isHeaderRow = true;
fs.createReadStream('./data/s-and-p-500-companies.csv', 'utf8')
.pipe(new CsvReadableStream({ trim: true }))
.on('data', function (row) {
if (isHeaderRow === true) {
isHeaderRow = false;
} else {
stocks.push({ ticker: row[0], companyName: row[1] });
}
})
.on('end', function () {
resolve(stocks);
});
Copy link
Author

Choose a reason for hiding this comment

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

🔴 Missing error handler on file stream causes promise to hang forever if CSV file is unreadable

The loadCsvData function creates a Promise that only calls resolve on the stream's end event, but never handles the stream's error event. If the CSV file doesn't exist or can't be read, fs.createReadStream will emit an error event. Since there's no .on('error', ...) handler and no reject call, the Promise will never settle.

Root Cause and Impact

The Promise constructor at src/data-loader/load-csv-data.ts:6 only destructures resolve but not reject:

const promise = new Promise<Stock[]>((resolve) => {

And there is no .on('error', ...) handler on the stream. If the file ./data/s-and-p-500-companies.csv is missing or unreadable (e.g., the working directory is wrong, file permissions issue, or Docker misconfiguration), the stream emits an unhandled error event. This causes:

  1. The Promise never resolves or rejects
  2. StocksService.stocks remains a pending Promise forever (src/stocks/stocks.service.ts:10)
  3. All requests to /stocks or /stocks/:ticker will hang indefinitely since they await this.stocks
  4. The unhandled stream error may also crash the Node.js process

Expected: The promise should reject on error so the application can fail fast with a clear error message.

Suggested change
const promise = new Promise<Stock[]>((resolve) => {
const stocks: Stock[] = [];
let isHeaderRow = true;
fs.createReadStream('./data/s-and-p-500-companies.csv', 'utf8')
.pipe(new CsvReadableStream({ trim: true }))
.on('data', function (row) {
if (isHeaderRow === true) {
isHeaderRow = false;
} else {
stocks.push({ ticker: row[0], companyName: row[1] });
}
})
.on('end', function () {
resolve(stocks);
});
const promise = new Promise<Stock[]>((resolve, reject) => {
const stocks: Stock[] = [];
let isHeaderRow = true;
fs.createReadStream('./data/s-and-p-500-companies.csv', 'utf8')
.on('error', function (err) {
reject(err);
})
.pipe(new CsvReadableStream({ trim: true }))
.on('data', function (row) {
if (isHeaderRow === true) {
isHeaderRow = false;
} else {
stocks.push({ ticker: row[0], companyName: row[1] });
}
})
.on('end', function () {
resolve(stocks);
});
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +18 to +23
it('/ (GET)', () => {
return request(app.getHttpServer())
.get('/')
.expect(200)
.expect('Hello World!');
});
Copy link
Author

Choose a reason for hiding this comment

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

🟡 E2E test expects non-existent root route returning 'Hello World!'

The e2e test at test/app.e2e-spec.ts:18-22 sends GET / and expects a 200 response with body 'Hello World!', but the application has no root route handler.

Root Cause and Impact

The AppModule (src/app.module.ts:5-7) only imports StocksModule and HealthModule, which register routes at /stocks, /stocks/:ticker, and /health. There is no controller handling GET /.

The test:

it('/ (GET)', () => {
  return request(app.getHttpServer())
    .get('/')
    .expect(200)
    .expect('Hello World!');
});

This will receive a 404 response from NestJS instead of the expected 200 with 'Hello World!'. This appears to be a leftover from the default NestJS project scaffold that was not updated when the app controller was removed.

Impact: The e2e test suite will always fail.

Suggested change
it('/ (GET)', () => {
return request(app.getHttpServer())
.get('/')
.expect(200)
.expect('Hello World!');
});
it('/stocks (GET)', () => {
return request(app.getHttpServer())
.get('/stocks')
.expect(200);
});
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant