-
Notifications
You must be signed in to change notification settings - Fork 0
Implement Local Log push and SQL Execution Support #9
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: master
Are you sure you want to change the base?
Implement Local Log push and SQL Execution Support #9
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 implements integration with external DevTools for local development and production monitoring. It adds support for remote log pushing, SQL query execution for debugging, and DevTools connection handshake mechanisms.
- Added new system routes for API documentation (
GET/api/doc), SQL execution (POST/api/execute-sql), and DevTools connection (POST/api/connect-devtools) - Implemented CORS support to allow DevTools running on different origins to communicate with the API
- Extended logging to support remote batch sending in both development and production environments with dynamic token configuration
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 29 comments.
Show a summary per file
| File | Description |
|---|---|
| types/router.d.ts | Added ReservedRoutes type to mark system routes as deprecated when overridden |
| db/mod.ts | Extracted Sql type definition for reuse in other modules |
| api/server.ts | Added CORS headers handling for DevTools integration in development mode |
| api/router.ts | Modified signature to accept RouterOptions and automatically register system routes |
| api/log.ts | Extended logger to support dynamic token updates and remote log batching in dev mode |
| api/env.ts | Renamed DEVTOOL_TOKEN to DEVTOOL_REPORT_TOKEN and added DEVTOOL_ACCESS_TOKEN |
| api/doc.ts | New file implementing automatic API documentation generation from route definitions |
| api/dev.ts | New file implementing SQL execution and DevTools handshake routes |
| README.md | Updated documentation to reflect renamed environment variables |
| .github/workflows/ga-npm-publish.yml | Changed quote style from double to single quotes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ): (ctx: RequestContext) => Awaitable<Response> => { | ||
| const routeMaps: Record<string, Route> = Object.create(null) | ||
|
|
||
| if (!defs['POST/api/execute-sql']) { |
Copilot
AI
Jan 5, 2026
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.
The route is being added even if SQL is not provided, which will result in the route returning NotImplemented responses. Consider only adding the route when SQL is actually configured to avoid exposing a non-functional endpoint.
| if (!defs['POST/api/execute-sql']) { | |
| if (sql && !defs['POST/api/execute-sql']) { |
| res.headers.set('Access-Control-Allow-Origin', origin) | ||
| res.headers.set('Access-Control-Allow-Methods', 'POST, OPTIONS') | ||
| res.headers.set('Access-Control-Allow-Headers', 'Authorization, Content-Type') | ||
| res.headers.set('Access-Control-Allow-Credentials', 'true') |
Copilot
AI
Jan 5, 2026
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.
These lines have inconsistent indentation (extra leading spaces) compared to the rest of the function. The indentation should match the surrounding code.
| const res = respond.NoContent() | ||
| return addCorsHeaders(res, origin) |
Copilot
AI
Jan 5, 2026
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.
This line has inconsistent indentation (extra leading spaces). The indentation should match the surrounding code.
| const res = respond.NoContent() | |
| return addCorsHeaders(res, origin) | |
| const res = respond.NoContent() | |
| return addCorsHeaders(res, origin) |
| type LoggerOptions = | ||
| { |
Copilot
AI
Jan 5, 2026
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.
The type definition spans multiple lines with unusual formatting. The opening brace should be on the same line as the type name for consistency with TypeScript conventions.
| type LoggerOptions = | |
| { | |
| type LoggerOptions = { |
| export type Sql = < | ||
| T extends { [k in string]: unknown } | undefined, | ||
| P extends BindValue | BindParameters | undefined, | ||
| >(sqlArr: TemplateStringsArray, ...vars: unknown[]) => { | ||
| get: (params?: P) => T | undefined | ||
| all: (params?: P) => T[] | ||
| run: (params?: P) => void | ||
| value: (params?: P) => T[keyof T][] | undefined | ||
| } |
Copilot
AI
Jan 5, 2026
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.
The type definition allows T to be undefined, but the return types don't properly handle this case. For example, value: (params?: P) => T[keyof T][] | undefined would fail at compile time if T is undefined because undefined has no keys. The type constraint should either exclude undefined or the return types should be adjusted.
| - name: 🟢 Update node | ||
| uses: actions/setup-node@v6 | ||
| with: { node-version: 24, registry-url: "https://registry.npmjs.org" } | ||
| with: { node-version: 24, registry-url: 'https://registry.npmjs.org' } |
Copilot
AI
Jan 5, 2026
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.
The quote style was changed from double quotes to single quotes. While this change is cosmetic, it should be consistent with the project's style guidelines. Ensure this aligns with the project's formatting standards.
| with: { node-version: 24, registry-url: 'https://registry.npmjs.org' } | |
| with: { node-version: 24, registry-url: "https://registry.npmjs.org" } |
| // Dev convenience for local devtools development | ||
| res.headers.set('Access-Control-Allow-Origin', origin) | ||
| res.headers.set('Access-Control-Allow-Methods', 'POST, OPTIONS') | ||
| res.headers.set('Access-Control-Allow-Headers', 'Authorization, Content-Type') | ||
| res.headers.set('Access-Control-Allow-Credentials', 'true') |
Copilot
AI
Jan 5, 2026
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.
The comment mentions "Dev convenience for local devtools development" but uses inconsistent indentation (extra space before the comment). This should align with the surrounding code indentation.
| // Dev convenience for local devtools development | |
| res.headers.set('Access-Control-Allow-Origin', origin) | |
| res.headers.set('Access-Control-Allow-Methods', 'POST, OPTIONS') | |
| res.headers.set('Access-Control-Allow-Headers', 'Authorization, Content-Type') | |
| res.headers.set('Access-Control-Allow-Credentials', 'true') | |
| // Dev convenience for local devtools development | |
| res.headers.set('Access-Control-Allow-Origin', origin) | |
| res.headers.set('Access-Control-Allow-Methods', 'POST, OPTIONS') | |
| res.headers.set('Access-Control-Allow-Headers', 'Authorization, Content-Type') | |
| res.headers.set('Access-Control-Allow-Credentials', 'true') |
|
|
||
|
|
Copilot
AI
Jan 5, 2026
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.
There is trailing whitespace at the end of this line. This should be removed.
|
|
||
| const ev = `${makePrettyTimestamp(level, event)} ${callChain}`.trim() | ||
| props ? console[level](ev, props) : console[level](ev) | ||
|
|
Copilot
AI
Jan 5, 2026
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.
There is trailing whitespace at the end of this line. This should be removed.
| // @ts-ignore: Attach hidden method for dev tools | ||
| globalThis.__DEVTOOLS_SET_TOKEN__ = setToken |
Copilot
AI
Jan 5, 2026
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.
Attaching a hidden global method via globalThis creates a non-standard API surface that bypasses normal TypeScript type checking. This pattern makes the code harder to maintain and test. Consider using a more explicit dependency injection pattern or a proper configuration mechanism.
No description provided.