Repository navigation
Conversation
Migrate runtime target from Node.js 18-22 to 20-24 LTS. Updated Dockerfile to node:24-bookworm-slim, CI matrix to 20/22/24, engines to >=20.0.0, and all workflow pinned versions to 24. Removed outdated better-sqlite3 incompatibility warning (v12.8.0 supports Node 24). Also fixed copilot-usage.test.mjs hardcoded quota_reset_date that caused staleAfterReset to override remainingPercentage.
…ecutors - Reword comment in chatCore.ts to avoid `\bany\b` budget check match - Replace `any` with `JsonRecord` / `ProviderCredentials` / `unknown` in github.ts - Replace `any` with `ProviderCredentials` / `unknown` in cloudflare-ai.ts - Remove unreachable `credentials.accountId` access (not in ProviderCredentials type)
- analytics-tabs.spec.ts: replace hardcoded Korean aria-label with English
'Select time range' matching actual TimeRangeSelector component
- Toggle.tsx: add data-testid prop support to Toggle component
- ProxyTab.tsx: add data-testid='debug-mode-toggle' to debug toggle
- AppearanceTab.tsx: add data-testid='sidebar-toggle-{id}' to sidebar toggles
- settings-toggles.spec.ts: use data-testid selectors, fix sidebar test tab
navigation from 'General' to 'Appearance', add waitForTimeout after clicks
Synced 30 i18n README.md files (18–22 LTS → 20–24 LTS) and 30 i18n CLI-TOOLS.md files (Node.js 18+ → 20+) with the Node 24 migration. Fixed Czech localized warning text that was missed by the batch sed.
Added missing bug fix entries to CHANGELOG [Unreleased]: - Explicit any budget fixes (github.ts, cloudflare-ai.ts, chatCore.ts) - Typecheck fix for responseTranslator.ts choices cast - E2E test selector fixes with Playwright native API migration Fixed CONTRIBUTING sidebar tab location (General → Appearance).
Replaced CSS attribute selectors with Playwright's getByTestId/getByLabel/
getByRole APIs. Replaced waitForLoadState("networkidle") with "domcontentloaded".
Removed waitForTimeout() calls in favor of Playwright auto-waiting assertions.
8/8 E2E tests pass against fresh Docker build.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge OverviewThis incremental review covers only the changes made since the previous review at commit
Incremental Changes (1 file)Only
Files Reviewed (incremental diff only)
All other files in this PR were already reviewed in the previous review cycle. No new issues identified. All checks passing: Lint, Security Audit, Build, Unit Tests (1191/1191), Coverage, E2E (8/8), Integration, Security, docs-sync, any-budget |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the project to Node.js 24, updating the runtime target, Docker images, and documentation across multiple languages. It also addresses several technical debt items, including the removal of explicit any types in favor of stricter TypeScript interfaces, fixing hardcoded dates in unit tests, and enhancing E2E test stability through the use of native Playwright locators and data-testid attributes. The review feedback identifies several instances where property access on Record<string, unknown> types will cause TypeScript errors in strict mode and suggests using index access or more specific interfaces to resolve these issues.
| if (responseFormat.type === "json_object") { | ||
| formatInstruction = | ||
| "Respond only with valid JSON. Do not include any text before or after the JSON object."; | ||
| } else if (responseFormat.type === "json_schema" && responseFormat.json_schema) { | ||
| const jsonSchema = responseFormat.json_schema as JsonRecord; | ||
| formatInstruction = `Respond only with valid JSON matching this schema:\n${JSON.stringify( | ||
| responseFormat.json_schema.schema, | ||
| jsonSchema.schema, | ||
| null, | ||
| 2 | ||
| )}\nDo not include any text before or after the JSON.`; | ||
| } |
There was a problem hiding this comment.
Accessing properties using dot notation on a Record<string, unknown> (via JsonRecord) will trigger TypeScript errors in strict mode (e.g., Property 'type' does not exist on type 'JsonRecord'). Since the goal is to remove any, you should use index access or define a more specific interface for the expected structure of responseFormat.
| if (responseFormat.type === "json_object") { | |
| formatInstruction = | |
| "Respond only with valid JSON. Do not include any text before or after the JSON object."; | |
| } else if (responseFormat.type === "json_schema" && responseFormat.json_schema) { | |
| const jsonSchema = responseFormat.json_schema as JsonRecord; | |
| formatInstruction = `Respond only with valid JSON matching this schema:\n${JSON.stringify( | |
| responseFormat.json_schema.schema, | |
| jsonSchema.schema, | |
| null, | |
| 2 | |
| )}\nDo not include any text before or after the JSON.`; | |
| } | |
| if (responseFormat['type'] === "json_object") { | |
| formatInstruction = | |
| "Respond only with valid JSON. Do not include any text before or after the JSON object."; | |
| } else if (responseFormat['type'] === "json_schema" && responseFormat['json_schema']) { | |
| const jsonSchema = responseFormat['json_schema'] as JsonRecord; | |
| formatInstruction = `Respond only with valid JSON matching this schema:\n${JSON.stringify( | |
| jsonSchema['schema'], | |
| null, | |
| 2 | |
| )}\nDo not include any text before or after the JSON.`; | |
| } |
| const systemIdx = messages.findIndex((m) => m.role === "system"); | ||
| if (systemIdx >= 0) { | ||
| return messages.map((m: any, i: number) => | ||
| return messages.map((m, i) => | ||
| i === systemIdx ? { ...m, content: `${m.content}\n\n${formatInstruction}` } : m | ||
| ); | ||
| } |
There was a problem hiding this comment.
The properties role and content cannot be accessed directly on JsonRecord (which is Record<string, unknown>). Use index access to maintain type safety without any.
| const systemIdx = messages.findIndex((m) => m.role === "system"); | |
| if (systemIdx >= 0) { | |
| return messages.map((m: any, i: number) => | |
| return messages.map((m, i) => | |
| i === systemIdx ? { ...m, content: `${m.content}\n\n${formatInstruction}` } : m | |
| ); | |
| } | |
| const systemIdx = messages.findIndex((m) => m['role'] === "system"); | |
| if (systemIdx >= 0) { | |
| return messages.map((m, i) => | |
| i === systemIdx ? { ...m, content: `${m['content']}\n\n${formatInstruction}` } : m | |
| ); | |
| } |
| if (modifiedBody.response_format && modelLower.includes("claude")) { | ||
| modifiedBody.messages = this.injectResponseFormat( | ||
| modifiedBody.messages, | ||
| modifiedBody.response_format | ||
| modifiedBody.messages as JsonRecord[], | ||
| modifiedBody.response_format as JsonRecord | ||
| ); | ||
| delete modifiedBody.response_format; | ||
| } |
There was a problem hiding this comment.
Dot access on modifiedBody (cast as JsonRecord) will cause TypeScript errors. Use index access for response_format and messages.
| if (modifiedBody.response_format && modelLower.includes("claude")) { | |
| modifiedBody.messages = this.injectResponseFormat( | |
| modifiedBody.messages, | |
| modifiedBody.response_format | |
| modifiedBody.messages as JsonRecord[], | |
| modifiedBody.response_format as JsonRecord | |
| ); | |
| delete modifiedBody.response_format; | |
| } | |
| if (modifiedBody['response_format'] && modelLower.includes("claude")) { | |
| modifiedBody['messages'] = this.injectResponseFormat( | |
| modifiedBody['messages'] as JsonRecord[], | |
| modifiedBody['response_format'] as JsonRecord | |
| ); | |
| delete modifiedBody['response_format']; | |
| } |
| } | ||
|
|
||
| const choice = isChoicesArray ? openaiResponse.choices[0] : null; | ||
| const choices = openaiResponse.choices as unknown[]; |
There was a problem hiding this comment.
If openaiResponse is typed as a Record<string, unknown> (or similar non-any type), accessing .choices via dot notation will result in a TypeScript error. Use index access instead.
| const choices = openaiResponse.choices as unknown[]; | |
| const choices = openaiResponse['choices'] as unknown[]; |
| stream: boolean, | ||
| credentials: ProviderCredentials | ||
| ): unknown { | ||
| void stream; | ||
| void credentials; |
There was a problem hiding this comment.
Instead of using void to suppress unused variable warnings, it is more idiomatic in this codebase to prefix unused parameters with an underscore.
| stream: boolean, | |
| credentials: ProviderCredentials | |
| ): unknown { | |
| void stream; | |
| void credentials; | |
| _stream: boolean, | |
| _credentials: ProviderCredentials | |
| ): unknown { |
There was a problem hiding this comment.
Pull request overview
Updates the project’s supported runtime range to Node.js 20–24 (dropping Node 18), refreshes CI/Docker/tooling accordingly, and aligns docs and E2E tests with the new runtime and Playwright selector approach.
Changes:
- Bump runtime support to Node.js 20–24 across Docker, CI workflows,
package.jsonengines, and CLI tooling. - Improve E2E test robustness by migrating to Playwright native locators (
getBy*) and addingdata-testidsupport to theTogglecomponent. - Sync core and i18n documentation to the updated Node.js requirements and adjust related tooling/tests for Node 24 type checking.
Reviewed changes
Copilot reviewed 82 out of 84 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/copilot-usage.test.mjs | Updates Copilot usage fixtures (incl. reset date). |
| tests/e2e/settings-toggles.spec.ts | Migrates settings toggle E2E selectors to getByRole/getByTestId and adjusts load waits. |
| tests/e2e/analytics-tabs.spec.ts | Switches to domcontentloaded waits and uses getByLabel for TimeRangeSelector. |
| src/shared/components/Toggle.tsx | Adds optional data-testid prop and forwards it to the switch element. |
| src/app/(dashboard)/dashboard/settings/components/ProxyTab.tsx | Adds data-testid for Debug Mode toggle. |
| src/app/(dashboard)/dashboard/settings/components/AppearanceTab.tsx | Adds data-testid for sidebar visibility toggles. |
| README.md | Updates documented runtime range to Node.js 20–24. |
| package.json | Drops Node 18 support via engines.node: >=20.0.0. |
| package-lock.json | Updates lockfile entries (incl. @omniroute/open-sse version). |
| open-sse/handlers/responseTranslator.ts | Adjusts typing for openaiResponse.choices to satisfy stricter TS checks. |
| open-sse/handlers/chatCore.ts | Rewords a comment to avoid triggering any-budget regex. |
| open-sse/executors/github.ts | Removes any usage by introducing ProviderCredentials/JsonRecord/unknown. |
| open-sse/executors/gemini-cli.ts | Updates fingerprint header to reflect Node 24. |
| open-sse/executors/cloudflare-ai.ts | Tightens credentials typing and removes unused accountId fallback. |
| docs/i18n/zh-CN/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/zh-CN/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/vi/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/vi/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/uk-UA/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/uk-UA/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/th/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/th/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/sv/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/sv/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/sk/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/sk/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/ru/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/ru/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/ro/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/ro/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/pt/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/pt/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/pt-BR/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/pt-BR/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/pl/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/pl/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/phi/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/phi/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/no/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/no/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/nl/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/nl/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/ms/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/ms/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/ko/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/ko/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/ja/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/ja/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/it/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/it/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/in/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/in/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/id/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/id/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/hu/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/hu/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/he/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/he/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/fr/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/fr/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/fi/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/fi/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/es/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/es/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/de/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/de/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/da/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/da/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/cs/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/cs/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/bg/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/bg/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/i18n/ar/README.md | Syncs Node runtime documentation to 20–24. |
| docs/i18n/ar/CLI-TOOLS.md | Updates CLI tools Node requirement to 20+. |
| docs/CLI-TOOLS.md | Updates Node requirement and cleans up tables. |
| Dockerfile | Bumps base image from Node 22 to Node 24. |
| docker-compose.yml | Updates example host Node path reference to Node 24. |
| CONTRIBUTING.md | Updates recommended Node version and settings tab naming. |
| CHANGELOG.md | Adds Unreleased notes covering Node 24 migration and related fixes. |
| bin/omniroute.mjs | Updates Node version warning logic to require Node >=20 and recommend Node 24. |
| .github/workflows/npm-publish.yml | Uses Node 24 for publish workflow. |
| .github/workflows/electron-release.yml | Uses Node 24 for Electron release workflow. |
| .github/workflows/ci.yml | Updates CI matrices and pins several jobs to Node 24. |
| .github/ISSUE_TEMPLATE/bug_report.yml | Updates Node version placeholder example to 24.0.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const debugToggle = page.getByTestId("debug-mode-toggle"); | ||
| await expect(debugToggle).toBeVisible({ timeout: 10000 }); | ||
|
|
||
| const initialState = await debugToggle.isChecked(); | ||
| await debugToggle.click(); | ||
| await expect(debugToggle).not.toBeChecked({ timeout: 5000 }); | ||
| }); |
There was a problem hiding this comment.
This assertion assumes the Debug Mode toggle should end up unchecked after a click, but debugMode defaults to false for a fresh settings store (so clicking should make it checked). To avoid a state-dependent E2E, capture the initial checked state and assert it flips after the click.
| await page.getByRole("tab", { name: "Advanced" }).click(); | ||
|
|
||
| await expect(debugToggle).toBeVisible({ timeout: 5000 }); | ||
| const debugToggle = page.getByTestId("debug-mode-toggle"); | ||
| await expect(debugToggle).toBeVisible({ timeout: 10000 }); | ||
|
|
||
| const wasChecked = await debugToggle.isChecked(); | ||
| await debugToggle.click(); | ||
| await expect(debugToggle).not.toBeChecked({ timeout: 5000 }); | ||
|
|
||
| await page.reload(); | ||
| await page.waitForLoadState("networkidle"); | ||
| await page.click("text=Advanced"); | ||
| await expect(debugToggle).not.toBeChecked({ timeout: 5000 }); | ||
| await page.waitForLoadState("domcontentloaded"); | ||
|
|
||
| await page.getByRole("tab", { name: "Advanced" }).click(); | ||
| await expect(debugToggle).toBeVisible({ timeout: 10000 }); | ||
| await expect(debugToggle).not.toBeChecked({ timeout: 10000 }); | ||
| }); |
There was a problem hiding this comment.
The persistence test has the same state assumption: after clicking, it always expects the toggle to be unchecked before/after reload. To make this reliable, assert that the value after reload matches the value immediately after the click (or that it toggles relative to the initial state).
|
Hi @dt418! Thanks for the Node.js 24 migration work 🎉 The PR has extensive merge conflicts with our recent i18n documentation restructuring (we moved CLI-TOOLS.md into We'll integrate the key Node 24 migration changes (Dockerfile, CI, engines, E2E test updates) directly on the release branch, with proper attribution. Closing this PR due to the conflicts, but your contribution is appreciated and will be credited in the v3.4.4 changelog. Thank you! 🚀 |
|
Thank you |
Summary
no-explicit-anyviolations and TypeScript type errors exposed by Node 24 type checkinggetByTestId,getByLabel,getByRole)Changes
Node 24 Migration (commit 1)
node:24-bookworm-slimbase imagesengines.node: >=20.0.0(dropped<24.0.0ceiling)Bug Fixes (commits 2–4)
anyfromgithub.ts,cloudflare-ai.ts; deadcredentials?.accountIdremovedchatCore.tscomment to pass\bany\bregex budget checkTS18046inresponseTranslator.ts(openaiResponse.choicesasunknown[])quota_reset_dateE2E Test Improvements (commit 7)
data-testidprop added toTogglecomponent (optional, backward-compatible)getByTestId()/getByLabel()/getByRole()waitForLoadState("networkidle")with"domcontentloaded"waitForTimeout()in favor of Playwright auto-waitingi18n Docs Sync (commit 5)
docs/i18n/*/README.mdfiles:Node.js 18–22 LTS→Node.js 20–24 LTSdocs/i18n/*/CLI-TOOLS.mdfiles:Node.js 18+→Node.js 20+cs) localized text missed by batch sedCI Status
All checks passing on branch:
Test Plan
npm run lint— 0 errorsnpm run test:unit— 1191/1191 passnpm run check:any-budget:t11— all files OKnode scripts/check-docs-sync.mjs— PASSNode.js 18references