-
Notifications
You must be signed in to change notification settings - Fork 0
claude sonnet 4 refactor run #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the backend into a layered architecture, adds type-safe validation utilities, and centralizes Express app creation with graceful shutdown handling.
- Introduces
ValidationError
and stream/partition validation insrc/utils/validation.ts
- Extracts services (
StreamrService
,TopologyService
), a controller, and routes for/topology
- Implements an
app
factory, config object, and updatescrawlTopology
to acceptmaxConcurrent
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/utils/validation.ts | Adds ValidationError , validateStreamId , validatePartition , and createStreamPartId utilities |
src/types/index.ts | Defines shared interfaces (LocationData , Node , etc.) |
src/services/TopologyService.ts | Encapsulates topology crawling logic and formatting |
src/services/StreamrService.ts | Wraps the Streamr client lifecycle |
src/routes/index.ts | Extracts route definitions for /topology |
src/index.ts | Refactors entry point to use createApp , initialize services, and handle graceful shutdown |
src/app.ts | Centralizes Express middleware, routes, health, and error handling |
src/fetchLocationData.ts | Imports LocationData from shared types instead of redeclaring |
src/crawlTopology.ts | Adds maxConcurrent parameter and removes hardcoded constant |
src/controllers/TopologyController.ts | Implements the HTTP handler with input validation and error handling |
src/config/index.ts | Introduces a type-safe config object |
src/Topology.ts | Removes local Node interface in favor of shared type |
src/README.md | Documents the new layered architecture and endpoints |
Comments suppressed due to low confidence (1)
src/utils/validation.ts:11
- Add unit tests for
validateStreamId
,validatePartition
, andcreateStreamPartId
to ensure all validation branches (missing, invalid, and valid inputs) are covered.
export function validateStreamId(streamIdParam: string): StreamID {
return 0 | ||
} | ||
|
||
const partition = parseInt(partitionParam) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include the radix parameter when parsing integers to avoid unexpected bases: parseInt(partitionParam, 10)
.
const partition = parseInt(partitionParam) | |
const partition = parseInt(partitionParam, 10) |
Copilot uses AI. Check for mistakes.
router.get('/topology', (req, res) => topologyController.getTopology(req as any, res)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid casting req
to any
; instead, properly type the handler: e.g., router.get<{}, any, any, TopologyQueryParams>('/topology', topologyController.getTopology)
.
router.get('/topology', (req, res) => topologyController.getTopology(req as any, res)) | |
interface TopologyQueryParams { | |
// Define the expected query parameters here, e.g.: | |
// param1: string; | |
} | |
router.get<{}, any, any, TopologyQueryParams>('/topology', (req, res) => topologyController.getTopology(req, res)) |
Copilot uses AI. Check for mistakes.
async getNetworkNode(): Promise<NetworkNode> { | ||
return (this.client.getNode()).getNode() as unknown as NetworkNode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The double getNode()
call and unknown
cast reduces clarity; consider updating the SDK types or wrapping the result in a properly typed method to eliminate the unknown
cast.
async getNetworkNode(): Promise<NetworkNode> { | |
return (this.client.getNode()).getNode() as unknown as NetworkNode | |
private getTypedNetworkNode(): NetworkNode { | |
return this.client.getNode() as NetworkNode | |
} | |
async getNetworkNode(): Promise<NetworkNode> { | |
return this.getTypedNetworkNode() |
Copilot uses AI. Check for mistakes.
Testing how backend refactor worked with claude sonnet 4