Migrate runtime from Bun to modern Node/npm - #79
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR migrates the project from Bun to Node.js 24: CI, Docker, scripts, and runtime use Node/npm; Bun-specific APIs and tooling are replaced with Node equivalents; a local sqlite wrapper and Node HTTP server were added; tests moved from bun:test to Vitest with compatibility shims. Changes
Sequence DiagramsequenceDiagram
participant Client
participant NodeServer as "Node Server\n(startNodeServer)"
participant Bundler as "Bundling Router\n(bundleEntrypoint)"
participant AppRouter as "App Router\n(router.fetch)"
participant DB as "SQLite Wrapper\n(app/db/sqlite.ts)"
Client->>NodeServer: HTTP request
activate NodeServer
NodeServer->>Bundler: build Request & check bundling routes
alt bundling route matches
Bundler->>Bundler: bundleEntrypoint via esbuild
Bundler-->>Client: bundled response
else no bundling match
Bundler->>AppRouter: forward Request
activate AppRouter
AppRouter->>DB: prepare/query/run
activate DB
DB-->>AppRouter: rows/result
deactivate DB
AppRouter-->>Client: response (may use getFileResponse)
deactivate AppRouter
end
deactivate NodeServer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
4e4ea17 to
fae0c8c
Compare
|
@cursor, Upgrade us to the latest @epic-web/config and migrate us to oxlint instead of eslint (make sure to update references to "ESLint" and change them to "oxlint" as appropriate). Check out the CI failures and address those. Then commit, format, commit, and push. |
|
Summary
Testing
I did not perform manual GUI testing because Kent’s request was specifically about config/lint/CI migration work, and the repo’s full automated validation gate now passes on the updated branch.   |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
@cursor, it looks like the package-lock.json file is outdated (based on CI failures), please fix. |
|
Summary
Testing
|
|
@cursoragent, this is what I get in each of the github actions: Run npm ci Please fix! |
|
I’ve confirmed the lockfile issue is already fixed on the current branch tip:
Testing
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for all 4 issues found in the latest run.
- ✅ Fixed: Bundling routes use wildcard keys but exact lookup
- Added prefix-based matching for wildcard bundling routes before falling back to exact path matches.
- ✅ Fixed: Static files served with wrong MIME content type
- Passed through an undefined content type so createLazyFile can infer MIME types from the file path.
- ✅ Fixed: Release script references nonexistent Node npm package version
- Removed the npx node invocation and used the already-installed Node binary for the release script.
- ✅ Fixed: writeFile corrupts ArrayBufferView with offset or length
- Write now respects ArrayBufferView byte offsets and lengths to avoid writing extra data.
Preview (c99ae596d3)
diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml
--- a/.github/workflows/publish.yml
+++ b/.github/workflows/publish.yml
@@ -8,6 +8,9 @@
- 'v*'
pull_request: {}
+env:
+ FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true
+
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true
@@ -22,61 +25,82 @@
runs-on: ubuntu-22.04
steps:
- name: ⬇️ Checkout repo
- uses: actions/checkout@v4
+ uses: actions/checkout@v6
- - name: 📦 Setup Bun
- uses: oven-sh/setup-bun@v2
+ - name: 📦 Setup Node
+ uses: actions/setup-node@v6
+ with:
+ node-version: 24
+ cache: npm
+ - name: Opt actions into Node 24
+ run: echo "FORCE_JAVASCRIPT_ACTIONS_TO_NODE24=true" >> "$GITHUB_ENV"
+
- name: 📥 Install dependencies
- run: bun install --frozen-lockfile
+ run: npm ci
- name: 🔬 Lint
- run: bun run check:ci
+ run: npm run lint:check
typecheck:
name: ʦ TypeCheck
runs-on: ubuntu-22.04
steps:
- name: ⬇️ Checkout repo
- uses: actions/checkout@v4
+ uses: actions/checkout@v6
- - name: 📦 Setup Bun
- uses: oven-sh/setup-bun@v2
+ - name: 📦 Setup Node
+ uses: actions/setup-node@v6
+ with:
+ node-version: 24
+ cache: npm
+ - name: Opt actions into Node 24
+ run: echo "FORCE_JAVASCRIPT_ACTIONS_TO_NODE24=true" >> "$GITHUB_ENV"
+
- name: 📥 Install dependencies
- run: bun install --frozen-lockfile
+ run: npm ci
- name: 🔎 Type check
- run: bun run typecheck
+ run: npm run typecheck
test:
name: 🧪 Test
runs-on: ubuntu-22.04
steps:
- name: ⬇️ Checkout repo
- uses: actions/checkout@v4
+ uses: actions/checkout@v6
- - name: 📦 Setup Bun
- uses: oven-sh/setup-bun@v2
+ - name: 📦 Setup Node
+ uses: actions/setup-node@v6
+ with:
+ node-version: 24
+ cache: npm
+ - name: Opt actions into Node 24
+ run: echo "FORCE_JAVASCRIPT_ACTIONS_TO_NODE24=true" >> "$GITHUB_ENV"
+
- name: 📥 Install dependencies
- run: bun install --frozen-lockfile
+ run: npm ci
- name: 🧪 Run tests
- run: bun test
+ run: npm test
build:
name: 🐳 Build
runs-on: ubuntu-22.04
steps:
- name: ⬇️ Checkout repo
- uses: actions/checkout@v4
+ uses: actions/checkout@v6
+ - name: Opt actions into Node 24
+ run: echo "FORCE_JAVASCRIPT_ACTIONS_TO_NODE24=true" >> "$GITHUB_ENV"
+
- name: 🐳 Set up Docker Buildx
- uses: docker/setup-buildx-action@v3
+ uses: docker/setup-buildx-action@v4
- name: 🏗️ Build Docker image
- uses: docker/build-push-action@v5
+ uses: docker/build-push-action@v6
with:
context: .
file: ./Dockerfile
@@ -118,20 +142,23 @@
if: ${{ github.event_name == 'push' }}
steps:
- name: ⬇️ Checkout repo
- uses: actions/checkout@v4
+ uses: actions/checkout@v6
+ - name: Opt actions into Node 24
+ run: echo "FORCE_JAVASCRIPT_ACTIONS_TO_NODE24=true" >> "$GITHUB_ENV"
+
- name: 🐳 Set up Docker Buildx
- uses: docker/setup-buildx-action@v3
+ uses: docker/setup-buildx-action@v4
- name: 🔑 Login to Docker Hub
- uses: docker/login-action@v3
+ uses: docker/login-action@v4
with:
username: ${{ secrets.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_TOKEN }}
- name: 📝 Generate Docker metadata
id: meta
- uses: docker/metadata-action@v5
+ uses: docker/metadata-action@v6
with:
images: ${{ secrets.DOCKERHUB_USERNAME }}/mediarss
tags: |
@@ -143,7 +170,7 @@
type=semver,pattern={{major}},enable=${{ startsWith(github.ref, 'refs/tags/v') }}
- name: 🚀 Build and push Docker image
- uses: docker/build-push-action@v5
+ uses: docker/build-push-action@v6
with:
context: .
file: ./Dockerfile
diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
--- a/.github/workflows/release.yml
+++ b/.github/workflows/release.yml
@@ -20,18 +20,21 @@
steps:
- name: Checkout repository
- uses: actions/checkout@v4
+ uses: actions/checkout@v6
with:
fetch-depth: 0 # Fetch all history for tags
token: ${{ secrets.RELEASE_TOKEN }}
- - name: Setup Bun
- uses: oven-sh/setup-bun@v2
+ - name: Setup Node.js
+ uses: actions/setup-node@v6
with:
- bun-version: latest
+ node-version: '24'
+ cache: npm
+ env:
+ FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true
- name: Install dependencies
- run: bun install --frozen-lockfile
+ run: npm ci
- name: Configure Git
run: |
@@ -42,4 +45,4 @@
env:
GITHUB_TOKEN: ${{ secrets.RELEASE_TOKEN }}
GITHUB_REPOSITORY: ${{ github.repository }}
- run: bun scripts/release.ts ${{ inputs.semverType }}
+ run: node --import ./server/register-hooks.mjs ./scripts/release.ts ${{ inputs.semverType }}
diff --git a/.gitignore b/.gitignore
--- a/.gitignore
+++ b/.gitignore
@@ -1,7 +1,6 @@
# dependencies
/node_modules
-# we use bun
-package-lock.json
+# npm lockfile is committed
# dotenv environment variable files
.env
diff --git a/.oxlintrc.json b/.oxlintrc.json
new file mode 100644
--- /dev/null
+++ b/.oxlintrc.json
@@ -1,0 +1,7 @@
+{
+ "extends": ["./node_modules/@epic-web/config/oxlint-config.json"],
+ "rules": {
+ "import/consistent-type-specifier-style": "off",
+ "typescript/consistent-type-imports": "off"
+ }
+}
diff --git a/AGENTS.md b/AGENTS.md
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -1,18 +1,18 @@
-Default to using Bun instead of Node.js.
+Default to using Node.js and npm.
## Linting
-Always run `bun run lint` before you're done working to fix any lint issues.
+Always run `npm run lint` before you're done working to fix any lint issues.
## Formatting
-Always run `bun run format` before you're done working to fix any formatting issues.
+Always run `npm run format` before you're done working to fix any formatting issues.
## Commit Gate
Always run the full gate before committing:
-`bun run validate`
+`npm run validate`
Do not commit if any part of the gate fails.
@@ -331,36 +331,30 @@
If navigation regressions appear, debug the app code first (Link handling,
route registration, and state updates) before assuming a framework bug.
-## Bun
+## Node.js
-- Use `bun <file>` instead of `node <file>` or `ts-node <file>`
-- Use `bun test` instead of `jest` or `vitest`
-- Use `bun build <file.html|file.ts|file.css>` instead of `webpack` or `esbuild`
-- Use `bun install` instead of `npm install` or `yarn install` or `pnpm install`
-- Use `bun run <script>`
-- Bun automatically loads .env, so don't use dotenv.
-- Use `Bun.env` instead of `process.env` to access environment variables. Runtime changes to `process.env` may not be reflected in `Bun.env`, so always use `Bun.env` for consistency.
+- Use `node` for runtime execution and `npm` for package management.
+- Use `npm install` to add dependencies and update `package-lock.json`.
+- Use `npm run <script>` for project scripts.
+- Use `process.env` for environment variables.
## Build
-There is no build step. This is shipped as-is. Instead, we use Bun's built-in runtime typescript support and we do a runtime bundling of the client-side code in `server/bundling.ts`.
+There is no build step. This is shipped as-is. We use modern Node.js runtime TypeScript support for `.ts` files, a small Node hook for `.tsx` loading, and runtime bundling of the client-side code in `server/bundling.ts`.
## APIs
-- `Bun.serve()` supports WebSockets, HTTPS, and routes. Don't use `express`.
-- `bun:sqlite` for SQLite. Don't use `better-sqlite3`.
-- `Bun.redis` for Redis. Don't use `ioredis`.
-- `Bun.sql` for Postgres. Don't use `pg` or `postgres.js`.
+- Use the Node HTTP stack plus `remix/node-fetch-server` for the server entrypoint.
+- Use `node:sqlite` for SQLite.
- `WebSocket` is built-in. Don't use `ws`.
-- Prefer `Bun.file` over `node:fs`'s readFile/writeFile
-- Bun.$`ls` instead of execa.
+- Prefer Node's built-in `fs`, `fs/promises`, and web `File`/`Blob` APIs for file access.
## Testing
-Use `bun test` to run tests.
+Use `npm test` to run tests.
```ts#index.test.ts
-import { test, expect } from "bun:test";
+import { test, expect } from "vitest";
test("hello world", () => {
expect(1).toBe(1);
@@ -372,7 +366,7 @@
To test the application with sample media files, use the `local-test` directory:
```bash
-bun run dev:test
+npm run dev:testThis command:
diff --git a/Dockerfile b/Dockerfile
--- a/Dockerfile
+++ b/Dockerfile
@@ -1,6 +1,6 @@
syntax=docker/dockerfile:1
-FROM oven/bun:1 AS base
+FROM node:24-bookworm AS base
WORKDIR /app
Install FFmpeg for metadata editing
@@ -8,8 +8,8 @@
Install dependencies
FROM base AS install
-COPY package.json bun.lock ./
-RUN bun install --frozen-lockfile --production
+COPY package.json package-lock.json ./
+RUN npm ci
Final image
FROM base AS release
@@ -29,4 +29,4 @@
EXPOSE 22050
Run the application
-CMD ["bun", "run", "start"]
+CMD ["npm", "run", "start"]
diff --git a/README.md b/README.md
--- a/README.md
+++ b/README.md
@@ -20,6 +20,20 @@
router behavior expectations), see
docs/remix/README.md.
+## Development Tooling
+
+- Runtime: Node.js 24
+- Package manager: npm
+- Linter: oxlint
+- Formatter: Biome
+
+Common commands:
+
+- npm run dev
+- npm run lint
+- npm run format
+- npm run validate
+
Screenshots
Dashboard
diff --git a/app/cache/cache.ts b/app/cache/cache.ts
--- a/app/cache/cache.ts
+++ b/app/cache/cache.ts
@@ -1,4 +1,3 @@
-import { Database } from 'bun:sqlite'
import fs from 'node:fs'
import path from 'node:path'
import {
@@ -17,6 +16,7 @@
} from 'remix/data-schema'
import { getEnv } from '#app/config/env.ts'
import { sql } from '#app/db/sql.ts'
+import { Database } from '#app/db/sqlite.ts'
function ensureDirectoryExists(filePath: string): void {
const dir = path.dirname(filePath)
@@ -64,10 +64,13 @@
typeof Database.prototype.prepare<CacheRow, [string]>
| null = null
let _setStatement: ReturnType<
- typeof Database.prototype.prepare<void, [string, string, string]>
- typeof Database.prototype.prepare<
-
Record<string, unknown>, -
[string, string, string] -
| null = null
let _deleteStatement: ReturnType<
- typeof Database.prototype.prepare<void, [string]>
- typeof Database.prototype.prepare<Record<string, unknown>, [string]>
| null = null
function getGetStatement() {
@@ -81,20 +84,21 @@
function getSetStatement() {
if (!_setStatement) {
-
_setStatement = getCacheDb().prepare<void, [string, string, string]>( -
'INSERT OR REPLACE INTO cache (key, metadata, value) VALUES (?, ?, ?)', -
)
-
_setStatement = getCacheDb().prepare< -
Record<string, unknown>, -
[string, string, string] -
}
>('INSERT OR REPLACE INTO cache (key, metadata, value) VALUES (?, ?, ?)')
- return _setStatement
- return _setStatement!
}
function getDeleteStatement() {
if (!_deleteStatement) {
-
_deleteStatement = getCacheDb().prepare<void, [string]>(
-
}
_deleteStatement = getCacheDb().prepare<Record<string, unknown>, [string]>( 'DELETE FROM cache WHERE key = ?', )
- return _deleteStatement
- return _deleteStatement!
}
// Schema for validating cache entry metadata
@@ -214,7 +218,7 @@
// Escape LIKE special characters in prefix to prevent unintended matches
// _ matches any single character, % matches any sequence of characters
const escapedPrefix = prefix.replace(/[\%_]/g, '\$&')
- const statement = db.prepare<void, [string]>(
- const statement = db.prepare<Record<string, unknown>, [string]>(
'DELETE FROM cache WHERE key LIKE ? ESCAPE "\"',
)
const result = statement.run(${escapedPrefix}%)
diff --git a/app/client/admin/edit-route-paths.test.ts b/app/client/admin/edit-route-paths.test.ts
--- a/app/client/admin/edit-route-paths.test.ts
+++ b/app/client/admin/edit-route-paths.test.ts
@@ -1,4 +1,4 @@
-import { expect, test } from 'bun:test'
+import { expect, test } from 'vitest'
import {
getFeedDetailPath,
getFeedEditPath,
diff --git a/app/config/env.ts b/app/config/env.ts
--- a/app/config/env.ts
+++ b/app/config/env.ts
@@ -178,7 +178,7 @@
- Must be called before accessing env.
*/
export function initEnv(): Env {
- const parsed = parseSafe(EnvSchema, Bun.env)
-
const parsed = parseSafe(EnvSchema, process.env)
if (!parsed.success) {
console.error('❌ Invalid environment variables:')
diff --git a/app/db/bun-sqlite-adapter-compat.ts b/app/db/bun-sqlite-adapter-compat.ts
--- a/app/db/bun-sqlite-adapter-compat.ts
+++ b/app/db/bun-sqlite-adapter-compat.ts
@@ -1,4 +1,3 @@
-import type { Database as BunDatabase } from 'bun:sqlite'
import type {
AdapterCapabilityOverrides,
DatabaseAdapter,
@@ -16,6 +15,7 @@
compileBunSqliteStatement,
quoteIdentifier,
} from './bun-data-table-sql-compiler.ts'
+import type { Database as BunDatabase } from './sqlite.ts'
type BunStatementResult = {
changes: number
diff --git a/app/db/feed-analytics-events.test.ts b/app/db/feed-analytics-events.test.ts
--- a/app/db/feed-analytics-events.test.ts
+++ b/app/db/feed-analytics-events.test.ts
@@ -1,4 +1,4 @@
-import { expect, test } from 'bun:test'
+import { expect, test } from 'vitest'
import '#app/config/init-env.ts'
import {
createFeedAnalyticsEvent,
diff --git a/app/db/feed-analytics-events.ts b/app/db/feed-analytics-events.ts
--- a/app/db/feed-analytics-events.ts
+++ b/app/db/feed-analytics-events.ts
@@ -1,8 +1,8 @@
-import type { Database } from 'bun:sqlite'
import { generateId } from '#app/helpers/crypto.ts'
import { createMediaKey, normalizePath } from '#app/helpers/path-parsing.ts'
import { db } from './index.ts'
import { sql } from './sql.ts'
+import type { Database } from './sqlite.ts'
import type { AnalyticsEventType, AnalyticsFeedType } from './types.ts'
export type CreateFeedAnalyticsEventData = {
diff --git a/app/db/feeds.test.ts b/app/db/feeds.test.ts
--- a/app/db/feeds.test.ts
+++ b/app/db/feeds.test.ts
@@ -1,9 +1,9 @@
-import { Database } from 'bun:sqlite'
-import { expect, test } from 'bun:test'
import fs from 'node:fs'
import path from 'node:path'
+import { expect, test } from 'vitest'
import { migrate } from './migrations.ts'
import { sql } from './sql.ts'
+import { Database } from './sqlite.ts'
/**
- Creates a test database that will be automatically closed and deleted.
diff --git a/app/db/index.ts b/app/db/index.ts
--- a/app/db/index.ts
+++ b/app/db/index.ts
@@ -1,7 +1,7 @@
-import { Database } from 'bun:sqlite'
import fs from 'node:fs'
import path from 'node:path'
import { getEnv } from '#app/config/env.ts'
+import { Database } from './sqlite.ts'
function getDatabasePath(): string {
const envPath = getEnv().DATABASE_PATH
diff --git a/app/db/migrations.analytics.test.ts b/app/db/migrations.analytics.test.ts
--- a/app/db/migrations.analytics.test.ts
+++ b/app/db/migrations.analytics.test.ts
@@ -1,4 +1,4 @@
-import { expect, test } from 'bun:test'
+import { expect, test } from 'vitest'
import { sql } from './sql.ts'
import { createMigratedTestDatabase } from './test-database.ts'
diff --git a/app/db/migrations.ts b/app/db/migrations.ts
--- a/app/db/migrations.ts
+++ b/app/db/migrations.ts
@@ -1,5 +1,5 @@
-import type { Database } from 'bun:sqlite'
import { sql } from './sql.ts'
+import type { Database } from './sqlite.ts'
type Migration = {
version: number
diff --git a/app/db/sqlite.ts b/app/db/sqlite.ts
new file mode 100644
--- /dev/null
+++ b/app/db/sqlite.ts
@@ -1,0 +1,110 @@
+import {
- DatabaseSync,
- type SQLInputValue,
- type SQLOutputValue,
- type StatementResultingChanges,
- type StatementSync,
+} from 'node:sqlite'
+type DatabaseParameters = Array | Record<string, unknown>
+
+type NamedParameters = Record<string, SQLInputValue>
+
+function isNamedParameters(value: unknown): value is NamedParameters {
- if (!value || typeof value !== 'object' || Array.isArray(value)) {
-
return false - }
- if (value instanceof Date || value instanceof ArrayBuffer) {
-
return false - }
- if (ArrayBuffer.isView(value)) {
-
return false - }
- return Object.getPrototypeOf(value) === Object.prototype
+}
+function normalizeRunResult(result: StatementResultingChanges) {
- return {
-
changes: -
typeof result.changes === 'bigint' -
? Number(result.changes) -
: result.changes, -
lastInsertRowid: -
typeof result.lastInsertRowid === 'bigint' -
? Number(result.lastInsertRowid) -
: result.lastInsertRowid, - }
+}
+class PreparedStatement<TRow = Record<string, unknown>> {
- #statement: StatementSync
- constructor(statement: StatementSync) {
-
this.#statement = statement - }
- all(...params: Array): Array {
-
return this.#call('all', params) as Array<TRow> - }
- get(...params: Array): TRow | undefined {
-
return this.#call('get', params) as TRow | undefined - }
- run(...params: Array) {
-
return normalizeRunResult( -
this.#call('run', params) as StatementResultingChanges, -
) - }
- #call(method: 'all' | 'get' | 'run', params: Array) {
-
if (params.length === 1 && isNamedParameters(params[0])) { -
return this.#statement[method](params[0]) -
} -
return this.#statement[method](...(params as Array<SQLInputValue>)) - }
+}
+export class Database {
- #database: DatabaseSync
- constructor(path: string) {
-
this.#database = new DatabaseSync(path, { -
timeout: 5_000, -
}) - }
- run(sql: string, ...params: Array) {
-
const statement = this.prepare(sql) -
return statement.run(...params) - }
- exec(sql: string): void {
-
this.#database.exec(sql) - }
- query<
-
TRow = Record<string, unknown>, -
TParams extends DatabaseParameters = [], -
(sql: string): PreparedStatement {
-
return this.prepare<TRow, TParams>(sql) - }
- prepare<
-
TRow = Record<string, unknown>, -
_TParams extends DatabaseParameters = [], -
(sql: string): PreparedStatement {
-
return new PreparedStatement<TRow>(this.#database.prepare(sql)) - }
- close(): void {
-
this.#database.close() - }
+}
+export type StatementResult = ReturnType<PreparedStatement['run']>
+export type { SQLOutputValue }
diff --git a/app/db/test-database.ts b/app/db/test-database.ts
--- a/app/db/test-database.ts
+++ b/app/db/test-database.ts
@@ -1,7 +1,7 @@
-import { Database } from 'bun:sqlite'
import fs from 'node:fs'
import path from 'node:path'
import { migrate } from './migrations.ts'
+import { Database } from './sqlite.ts'
export function createMigratedTestDatabase(prefix: string): {
db: Database
diff --git a/app/helpers/analytics-request.test.ts b/app/helpers/analytics-request.test.ts
--- a/app/helpers/analytics-request.test.ts
+++ b/app/helpers/analytics-request.test.ts
@@ -1,4 +1,4 @@
-import { expect, test } from 'bun:test'
+import { expect, test } from 'vitest'
import {
getClientFingerprint,
getClientIp,
diff --git a/app/helpers/analytics-window.test.ts b/app/helpers/analytics-window.test.ts
--- a/app/helpers/analytics-window.test.ts
+++ b/app/helpers/analytics-window.test.ts
@@ -1,4 +1,4 @@
-import { expect, test } from 'bun:test'
+import { expect, test } from 'vitest'
import { parseAnalyticsWindowDays } from './analytics-window.ts'
function createRequest(search = ''): Request {
diff --git a/app/helpers/bundle-version.ts b/app/helpers/bundle-version.ts
--- a/app/helpers/bundle-version.ts
+++ b/app/helpers/bundle-version.ts
@@ -7,7 +7,7 @@
*
- The version is based on:
-
- The app version from package.json
-
-
- A hash of the bun.lock file (captures dependency changes)
-
-
-
- A hash of the package-lock.json file (captures dependency changes)
- This ensures cache invalidation when either:
-
- The app version is bumped (new release)
@@ -16,7 +16,7 @@
- The app version is bumped (new release)
- The version is computed once at module load time and cached.
*/
function computeVersion(): string {
-
- const rootDir = path.resolve(import.meta.dir, '..', '..')
-
const rootDir = path.resolve(import.meta.dirname, '..', '..')
// Get app version from package.json
const packageJsonPath = path.join(rootDir, 'package.json')
@@ -24,7 +24,7 @@
const appVersion = packageJson.version || '0.0.0'// Get hash of lock file (captures dependency changes)
- const lockFilePath = path.join(rootDir, 'bun.lock')
- const lockFilePath = path.join(rootDir, 'package-lock.json')
let lockHash = 'nolockfile'
if (fs.existsSync(lockFilePath)) {
const lockContent = fs.readFileSync(lockFilePath)
diff --git a/app/helpers/decode-path-param.test.ts b/app/helpers/decode-path-param.test.ts
--- a/app/helpers/decode-path-param.test.ts
+++ b/app/helpers/decode-path-param.test.ts
@@ -1,4 +1,4 @@
-import { expect, test } from 'bun:test'
+import { expect, test } from 'vitest'
import { decodePathParam } from './decode-path-param.ts'
test('decodePathParam decodes valid encoded segments', () => {
diff --git a/app/helpers/exec.ts b/app/helpers/exec.ts
new file mode 100644
--- /dev/null
+++ b/app/helpers/exec.ts
@@ -1,0 +1,34 @@
+import { execFile } from 'node:child_process'
+import { promisify } from 'node:util'
+
+const execFileAsync = promisify(execFile)
+
+export type ExecResult = {
- stdout: string
- stderr: string
- exitCode: number
+}
+export async function execCommand(
- command: string,
- args: string[],
+): Promise { - try {
-
const { stdout, stderr } = await execFileAsync(command, args, { -
encoding: 'utf8', -
}) -
return { stdout, stderr, exitCode: 0 } - } catch (error) {
-
const failure = error as NodeJS.ErrnoException & { -
stdout?: string -
stderr?: string -
code?: number | string -
} -
return { -
stdout: failure.stdout ?? '', -
stderr: failure.stderr ?? failure.message, -
exitCode: typeof failure.code === 'number' ? failure.code : 1, -
} - }
+}
diff --git a/app/helpers/feed-artwork-resolution.ts b/app/helpers/feed-artwork-resolution.ts
--- a/app/helpers/feed-artwork-resolution.ts
+++ b/app/helpers/feed-artwork-resolution.ts
@@ -3,6 +3,7 @@
import type { Feed } from '#app/db/types.ts'
import { extractArtwork } from '#app/helpers/artwork.ts'
import { getFeedArtworkPath } from '#app/helpers/feed-artwork.ts'
+import { getFileResponse } from '#app/helpers/node-file.ts'
import { generatePlaceholderSvg } from '#app/helpers/placeholder-svg.ts'
/**
@@ -19,13 +20,17 @@
// Priority 1: Uploaded artwork
const uploadedArtwork = await getFeedArtworkPath(feedId)
if (uploadedArtwork) {
-
const artworkFile = Bun.file(uploadedArtwork.path) -
return new Response(artworkFile.stream(), { -
headers: { -
'Content-Type': uploadedArtwork.mimeType, -
'Cache-Control': 'public, max-age=86400',
-
const response = await getFileResponse( -
uploadedArtwork.path, -
new Request('http://localhost'), -
{ -
cacheControl: 'public, max-age=86400', -
contentType: uploadedArtwork.mimeType, },
-
})
-
) -
if (response) { -
return response -
}
}
... diff truncated: showing 800 of 8119 lines
</details>
</details>
|
@cursoragent what is FORCE_JAVASCRIPT_ACTIONS_TO_NODE24? I think that's stupid. Remove it. |
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/helpers/feed-artwork.ts (1)
81-90:⚠️ Potential issue | 🟠 MajorPrevent data loss by using atomic file operations for artwork replacement.
The current code deletes existing artwork at line 82, then writes the new file at line 89. If the write fails, the artwork is lost permanently. Use atomic file operations: write to a temporary file first, then rename it to the target path to ensure the old artwork is only replaced if the new write succeeds.
Update the implementation to use
Bun.file()for file operations per project guidelines instead ofnode:fs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/helpers/feed-artwork.ts` around lines 81 - 90, The code deletes existing artwork with deleteFeedArtwork(feedId) then writes the new file directly to artworkPath (ARTWORK_DIR + `${feedId}.${fileType.ext}`), risking deletion on write failure; instead, write atomically by using Bun.file() to create a temporary file (e.g., artworkPath + `.tmp-${process.pid}-${Date.now()}`), write the file contents from file.arrayBuffer() into that temp file, fs/rename the temp to artworkPath (or use Bun's rename) only after the write succeeds, and remove the temp on error; ensure you stop using node:fs.promises and replace write operations with Bun.file() semantics so the original file is only replaced on successful rename.server/cli.ts (1)
50-55:⚠️ Potential issue | 🟠 MajorAwait
server.stop(true)in the graceful-shutdown callback.
AppServer.stop()is treated as async everywhere else in this PR. Leaving it floating here letsclose-with-gracefinish before the server has actually closed its sockets and can surface shutdown failures as unhandled rejections.Suggested fix
const closeListeners = closeWithGrace({ delay: 500 }, async ({ err }) => { if (err) { console.error('Error during shutdown:', err) } console.log(`\n\n${colorize('Shutting down...', 'crimson')}`) - server.stop(true) // true = close all idle connections immediately + await server.stop(true) // true = close all idle connections immediately })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/cli.ts` around lines 50 - 55, The graceful-shutdown callback passed to closeWithGrace calls server.stop(true) without awaiting it; change the callback to await server.stop(true) (or return await server.stop(true)) so the closeWithGrace flow waits for the server to finish closing, and wrap the await in a try/catch to log any errors (use closeListeners/closeWithGrace and the server.stop method names to locate the code).
🧹 Nitpick comments (10)
app/helpers/ffmpeg.ts (2)
252-262: Error handling is reasonable, but stderr extraction could be simplified.The error handling correctly extracts
stderrfrom theExecFileException. SinceexecFileAsyncthrows an error object that always includesstderrandstdoutproperties when the command fails, you could use a type assertion for cleaner code.♻️ Optional: Simplify stderr extraction
try { await execFileAsync('ffmpeg', ffmpegArgs, { windowsHide: true, }) - } catch (error) { - const stderr = - error && typeof error === 'object' && 'stderr' in error - ? String(error.stderr) - : String(error) + } catch (error: unknown) { + const stderr = (error as { stderr?: string }).stderr ?? String(error) throw new Error(`FFmpeg failed: ${stderr}`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/helpers/ffmpeg.ts` around lines 252 - 262, The catch block around execFileAsync can be simplified by asserting the thrown error as the ExecFileException (or a type with stderr) and reading error.stderr directly instead of the verbose typeof/object check; update the catch to cast the caught variable to ExecFileException (or use "as any") and set stderr = String((err as ExecFileException).stderr ?? err) before throwing the new Error, referencing the existing execFileAsync call and the local stderr variable in the ffmpeg handling code.
289-297: Consider adding a timeout to prevent indefinite hangs.If
ffmpeghangs or stalls (e.g., due to a misconfigured environment or broken binary), this check will block indefinitely. Adding a timeout provides a safety net.⏱️ Optional: Add timeout for availability check
export async function isFFmpegAvailable(): Promise<boolean> { try { await execFileAsync('ffmpeg', ['-version'], { windowsHide: true, + timeout: 5000, // 5 second timeout }) return true } catch { return false } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/helpers/ffmpeg.ts` around lines 289 - 297, The isFFmpegAvailable function can hang if ffmpeg stalls; modify isFFmpegAvailable to enforce a timeout (e.g., 5s) when calling execFileAsync('ffmpeg', ['-version']). Implement this by racing the execFileAsync promise against a timeout promise that rejects after the timeout or by using execFileAsync's timeout option if available, and ensure the catch treats timeout as a failure (return false) and cleans up any child process if needed.app/helpers/feed-artwork.ts (1)
24-31: AvoidexistsSyncin async flow and remove check-then-use race.Line [26] blocks the event loop and still races with Line [28]. Prefer a single async read attempt and only ignore
ENOENT.♻️ Suggested refactor
export async function getFeedArtworkPath( feedId: string, ): Promise<{ path: string; mimeType: string } | null> { for (const ext of ['jpg', 'jpeg', 'png', 'webp']) { const artPath = path.join(ARTWORK_DIR, `${feedId}.${ext}`) - if (fs.existsSync(artPath)) { - // Detect actual MIME type from file content - const fileType = await fileTypeFromFile(artPath) - const mimeType = fileType?.mime ?? 'application/octet-stream' - return { path: artPath, mimeType } - } + try { + const fileType = await fileTypeFromFile(artPath) + const mimeType = fileType?.mime ?? 'application/octet-stream' + return { path: artPath, mimeType } + } catch (error) { + const code = (error as NodeJS.ErrnoException).code + if (code !== 'ENOENT') throw error + } } return null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/helpers/feed-artwork.ts` around lines 24 - 31, Replace the blocking existsSync + fileTypeFromFile pattern in the function that looks up artwork (using feedId and ARTWORK_DIR) with a single async file access attempt: build artPath for each extension, call fs.promises.readFile or fs.promises.stat wrapped in try/catch (or use fs.promises.open) instead of existsSync, then pass the path or buffer to fileTypeFromFile; on errors only ignore when error.code === 'ENOENT' and rethrow other errors, and return { path: artPath, mimeType } when fileTypeFromFile succeeds (keeping the same mimeType fallback logic). Use the existing symbols feedId, ARTWORK_DIR, and fileTypeFromFile to locate where to change.app/helpers/media.ts (1)
968-974: Avoid serialstatcalls in the pre-pass.Line 970 currently awaits
statone file at a time. For large directories, this adds avoidable latency before metadata extraction.♻️ Proposed refactor (bounded parallel stats)
- const validFileStats: Array<{ path: string; mtime: number }> = [] - for (const p of filePaths) { - try { - const { mtimeMs } = await fs.promises.stat(p) - const mtime = Number(mtimeMs) - validFileStats.push({ path: p, mtime }) - } catch { - // File may have been deleted since scan - } - } + const validFileStats = ( + await Promise.all( + filePaths.map((p) => + metadataLimit(async () => { + try { + const { mtimeMs } = await fs.promises.stat(p) + return { path: p, mtime: Number(mtimeMs) } + } catch { + // File may have been deleted since scan + return null + } + }), + ), + ) + ).filter((entry): entry is { path: string; mtime: number } => entry !== null)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/helpers/media.ts` around lines 968 - 974, The current loop awaiting fs.promises.stat(p) one-by-one on filePaths causes serial IO; replace it with a bounded-parallel stat pass that collects results into validFileStats concurrently (e.g., map filePaths to stat promises and run with a concurrency limit via a simple worker pool or a p-limit-style utility), handle rejections per-file (skip or log) and push { path: p, mtime: Number(mtimeMs) } for successful stats; ensure you still reference filePaths, validFileStats, fs.promises.stat and mtimeMs when implementing the concurrent/stat collection logic.app/helpers/feed-artwork-resolution.ts (1)
23-30: Thread the caller'sRequestthrough artwork resolution.
new Request('http://localhost')dropsHEADplus validator/range headers from the original call, so uploaded artwork won't participate in the same conditional-file behavior as the route variants that pass the real request. Consider addingrequest: RequesttoresolveFeedArtwork(...)and forwarding it here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/helpers/feed-artwork-resolution.ts` around lines 23 - 30, The helper currently constructs a new Request('http://localhost') which drops HEAD/validator/range headers; update resolveFeedArtwork to accept a caller Request (add a request: Request parameter to resolveFeedArtwork) and forward that real request into the getFileResponse call instead of creating a new Request, so getFileResponse(uploadedArtwork.path, request, { cacheControl: ..., contentType: ... }) preserves conditional/file semantics; update all callers of resolveFeedArtwork to pass their incoming Request through.app/routes/admin/api/media-stream.ts (1)
41-51: Consider usingfileExistsfor consistency withmedia.ts.The
filevariable fromcreateLazyFileis created but never used—only its truthiness is checked before passingfilePathtoserveFileWithRanges. Themedia.tsroute usesfileExists(filePath)for this purpose, which is more direct.♻️ Suggested fix for consistency
-import { createLazyFile } from '#app/helpers/node-file.ts' +import { fileExists } from '#app/helpers/node-file.ts'// Confirm the file exists - const file = await createLazyFile(filePath) - if (!file) { + if (!(await fileExists(filePath))) { return new Response('File not found', { status: 404 }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/admin/api/media-stream.ts` around lines 41 - 51, Replace the unused createLazyFile check with the direct existence check used elsewhere: call fileExists(filePath) to verify the file before calling serveFileWithRanges, remove the unused file variable from createLazyFile, and keep serveFileWithRanges(filePath, context.request, 'private, max-age=3600') unchanged; this targets the file existence logic involving createLazyFile, fileExists, serveFileWithRanges and filePath.server/node-server.ts (1)
62-91: Consider extracting the shared close logic.The close-and-await pattern is duplicated between
[Symbol.asyncDispose]andstop. You could extract a private helper or have one call the other.♻️ Suggested refactor
+ const closeServer = async () => { + await new Promise<void>((resolve, reject) => { + server.close((error) => { + if (error) { + reject(error) + return + } + resolve() + }) + }) + } + [Symbol.dispose]: () => { server.closeIdleConnections?.() server.close() }, [Symbol.asyncDispose]: async () => { server.closeIdleConnections?.() - await new Promise<void>((resolve, reject) => { - server.close((error) => { - if (error) { - reject(error) - return - } - resolve() - }) - }) + await closeServer() }, stop: async (closeIdleConnections = true) => { if (closeIdleConnections) { server.closeIdleConnections?.() } - await new Promise<void>((resolve, reject) => { - server.close((error) => { - if (error) { - reject(error) - return - } - resolve() - }) - }) + await closeServer() },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/node-server.ts` around lines 62 - 91, Extract the duplicated close-and-await logic into a single helper (e.g., a private async function like closeServerAndAwait or reuse [Symbol.asyncDispose]) and have both [Symbol.asyncDispose] and stop call that helper; the helper should run server.closeIdleConnections?.(), then return a Promise that resolves/rejects based on server.close callback so the close behavior is centralized and not duplicated across stop and [Symbol.asyncDispose].Dockerfile (1)
7-7: Trim recommended OS packages from the runtime image.
apt-get installpulls Debian "recommended" packages by default. Add--no-install-recommendsto keep the image smaller and reduce the dependency surface.💡 Suggested fix
-RUN apt-get update && apt-get install -y ffmpeg && rm -rf /var/lib/apt/lists/* +RUN apt-get update && apt-get install -y --no-install-recommends ffmpeg && rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 7, The Dockerfile RUN line installs ffmpeg with apt-get which pulls Debian "recommended" packages; update the RUN command that contains "apt-get install -y ffmpeg" to include the flag "--no-install-recommends" so it becomes "apt-get install -y --no-install-recommends ffmpeg" (keeping the surrounding "apt-get update &&" and the trailing "rm -rf /var/lib/apt/lists/*") to reduce image size and dependency surface..github/workflows/publish.yml (1)
11-12: Redundant environment variable setting.
FORCE_JAVASCRIPT_ACTIONS_TO_NODE24is already set at the workflow level (lines 11-12), so the per-jobechosteps (lines 36-37, 58-59, 80-81, 96-97, 147-148) are redundant. The workflow-levelenvapplies to all jobs automatically.♻️ Remove redundant env echo steps
- name: 📥 Install dependencies run: npm ci - - name: Opt actions into Node 24 - run: echo "FORCE_JAVASCRIPT_ACTIONS_TO_NODE24=true" >> "$GITHUB_ENV" - - name: 🔬 Lint run: npm run lint:checkApply similar removal to the
typecheck,test,build, andpublishjobs.Also applies to: 36-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish.yml around lines 11 - 12, Remove the redundant per-job steps that echo the workflow-level env var FORCE_JAVASCRIPT_ACTIONS_TO_NODE24; since this variable is already set in the workflow env, delete the echo steps in the jobs typecheck, test, build, and publish (the steps that run commands like `echo "FORCE_JAVASCRIPT_ACTIONS_TO_NODE24=true"`), leaving the workflow-level env intact and ensuring no job-level duplication remains.server/bundling.ts (1)
170-173: Consider caching bundled output in production.Each request rebuilds the bundle from scratch. For production, consider memoizing bundle results keyed by filepath + mtime to avoid redundant esbuild invocations on repeated requests.
Also applies to: 206-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/bundling.ts` around lines 170 - 173, Requests currently rebuild bundles on every call to bundleEntrypoint (called with filepath and rootDir); add a production-safe memoization layer that caches the bundle output keyed by a stable key (e.g., filepath + file mtime or a file content hash) and returns the cached result when the key matches. Implement a simple in-memory Map cache in server/bundling.ts that maps the computed key to the bundle result, invalidate/update the cache when the mtime changes, and guard caching behind a NODE_ENV === "production" check; apply the same caching logic to the other bundleEntrypoint invocation referenced (lines ~206-209) so both call sites use the shared cache.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 348: The doc line "Use `node:sqlite` for SQLite." should be updated to
warn that `node:sqlite` (available unflagged in Node.js 24) is still
experimental (stability 1.1) and not recommended for production; either add a
short note in AGENTS.md stating its experimental status, Node.js version
requirement, and potential for breaking changes without semver guarantees, or
alternatively add/point to test coverage that validates SQLite operations
(mention tests that exercise SQLite behavior) to accept its use—update the
sentence to include this guidance and a brief recommendation for production
users.
In `@app/db/sqlite.ts`:
- Around line 29-40: normalizeRunResult currently converts bigint fields in
StatementResultingChanges to number which can lose precision for values >
Number.MAX_SAFE_INTEGER; change normalizeRunResult to avoid blind Number(...)
conversion by either (A) preserving bigint values when result.changes or
result.lastInsertRowid are bigints, (B) converting bigints to string when they
exceed Number.MAX_SAFE_INTEGER, or (C) perform a safe-range check (compare to
Number.MAX_SAFE_INTEGER) and only Number(...) when safe, otherwise return the
bigint/string; update the returned shape/type annotations accordingly so callers
of normalizeRunResult and any consumers of changes/lastInsertRowid handle bigint
or string values instead of assuming number.
In `@app/helpers/feed-artwork.ts`:
- Line 1: The project docs still mandate Bun-first APIs but this PR switches
app/helpers/feed-artwork.ts to use Node's "node:fs" import; update the
AGENTS/coding-guidelines doc to reflect the Node 24 migration by removing or
altering the "Use `bun <file>`" and "Prefer `Bun.file` over `node:fs`" rules,
add a clear statement that Node 24 and node:fs APIs are now preferred, and give
guidance on when Bun usage is acceptable (if at all); ensure the guideline
references the specific change (app/helpers/feed-artwork.ts using node:fs) so
future reviewers accept Node filesystem usage.
In `@app/helpers/node-file.ts`:
- Around line 42-45: getFileResponse currently forces 'application/octet-stream'
by calling createLazyFile(path, options.contentType ??
'application/octet-stream'), which prevents createLazyFile's extension-based
MIME detection; change the call in getFileResponse to pass options.contentType
directly (or options.contentType ?? undefined) instead of defaulting to
'application/octet-stream' so createLazyFile can run its detection logic when
contentType is omitted.
- Around line 63-66: The Blob branch currently buffers the entire file via
data.arrayBuffer(); instead, stream the Blob to disk using the blob.stream()
path to avoid OOM: create a Node writable with fs.createWriteStream(path),
convert the Blob's web ReadableStream to a Node Readable via
stream.Readable.fromWeb(data.stream()), and use the pipeline from
'stream/promises' (or stream.pipeline wrapped in a promise) to pipe the readable
into the write stream and await completion; update the code inside the if (data
instanceof Blob) block in node-file.ts (the Blob handling code) to implement
this streaming approach and remove the arrayBuffer() usage.
- Line 74: In the write path where fs.promises.writeFile(path, new
Uint8Array(data.buffer)) is used, fix handling of ArrayBufferView slices by
preserving byteOffset/byteLength: construct the view with those bounds (e.g.,
new Uint8Array(data.buffer, data.byteOffset, data.byteLength)) or use
Buffer.from(data.buffer, data.byteOffset, data.byteLength) before calling
fs.promises.writeFile so only the sliced portion of the ArrayBufferView named
data is written.
In `@app/helpers/range-request.ts`:
- Around line 4-7: The isMalformedRangeRequest guard is too strict and rejects
valid multi-range headers; update the function isMalformedRangeRequest to either
remove the prefilter entirely (letting Remix/createFileResponse handle Range
validation) or broaden its validation to allow comma-separated byte-range-specs
per RFC 7233 (i.e., accept multiple "start-end" segments separated by commas and
optional whitespace) so headers like "bytes=0-1,4-5" are not treated as
malformed. Locate isMalformedRangeRequest in app/helpers/range-request.ts and
implement one of these two fixes consistently with the project policy on
delegating range handling to Remix.
In `@app/helpers/version.ts`:
- Around line 1-6: This change replaced Bun runtime APIs with Node APIs
(execFile, readFile, path, execFileAsync), which conflicts with the repo's
current AGENTS/coding-guideline that enforces Bun-based usage; either update the
coding-guideline rules in the same PR to permit Node APIs (remove/adjust the Bun
enforcement) or revert the implementation to Bun APIs — replace
execFile/execFileAsync with Bun.spawn/Bun.spawnSync usage and replace readFile
usage with Bun.file(...).text() or Bun.readFile equivalents so the helpers
(references: execFile, execFileAsync, readFile, path) conform to the repository
policy.
In `@app/routes/admin/api/media.test.ts`:
- Around line 8-28: The mock function variables scanAllMediaRootsMock,
resolveMediaPathMock, and listMediaPopularityMetricsMock must be declared inside
a vi.hoisted() callback to avoid TDZ when vi.mock(...) factories run; move the
three vi.fn declarations into a single vi.hoisted(() => { ... }) block and keep
the same shapes/return values, then keep the existing vi.mock calls referencing
those hoisted variables so '#app/helpers/media.ts' uses scanAllMediaRootsMock,
'#app/config/env.ts' uses resolveMediaPathMock, and
'#app/db/feed-analytics-events.ts' uses listMediaPopularityMetricsMock.
In `@app/routes/art.ts`:
- Around line 126-137: The current handler converts a missing uploaded artwork
into a hard 404 by returning Response('File not found') when
getFileResponse(uploadedFeedArtwork.path, ...) returns falsy; instead remove
that hard return so the function falls through to the existing feed.imageUrl /
placeholder fallback logic (as resolveFeedArtwork does). Concretely, in the
block using getFileResponse and uploadedFeedArtwork.path, keep "if (response)
return response" but delete the subsequent "return new Response('File not
found', { status: 404 })" (or replace it with a non-returning noop / return
undefined) so the route continues to the feed.imageUrl / placeholder resolution
paths. Ensure changes reference getFileResponse, uploadedFeedArtwork,
resolveFeedArtwork and feed.imageUrl.
In `@app/routes/oauth/token.ts`:
- Around line 183-185: The allowedHosts parsing can produce empty tokens and
inadvertently create an empty allowlist; update the logic that builds
allowedHosts to split, trim, and filter out empty strings (e.g.,
.split(',').map(h=>h.trim()).filter(Boolean)), and if the resulting array is
empty set allowedHosts to undefined/null so the later host validation (the
host-check using allowedHosts) won’t treat an empty list as a deny-all; modify
the variable creation where allowedHosts is computed and ensure the downstream
host validation uses the adjusted allowedHosts value.
In `@index.ts`:
- Around line 15-18: Replace the filesystem URL construction using new URL('.',
import.meta.url).pathname with import.meta.dirname so rootDir holds a proper
filesystem path (not a percent-encoded or URL-style path); update the assignment
to rootDir (used by createBundlingRoutes and any consumers) to use
import.meta.dirname consistent with other files (e.g., app/helpers/version.ts)
to avoid Windows and encoding issues.
In `@scripts/release.ts`:
- Line 2: The packageJsonPath is being built from a file URL using URL.pathname
which breaks on Windows; change the code to import fileURLToPath from 'url' and
convert the file URL to a platform path (use fileURLToPath(packageJsonUrl)) when
assigning packageJsonPath (where packageJsonUrl and packageJsonPath are defined)
so subsequent readFile/writeFile calls use a correct OS path.
- Around line 44-53: The script updates packageJson.version then stages
package.json but doesn't update or stage package-lock.json, which causes
lockfile drift; after writing package.json (where packageJson.version is set via
writeFile) run a command to regenerate the lockfile (e.g., execCommand('npm',
['install','--package-lock-only']) or equivalent) and then stage
package-lock.json with execCommand('git', ['add', 'package-lock.json']) before
continuing with tagName and the rest of the Git operations so the lockfile
matches the bumped version.
In `@server/platform-cli.ts`:
- Around line 3-20: The spawnAndForget function currently swallows async
child-process 'error' events and returns synchronously; change it to an async
Promise-based API (e.g., export or rename spawnAndForget to return
Promise<boolean>) that attaches both an 'error' listener (resolve false) and a
'spawn' or 'close' listener (resolve true) on the spawned ChildProcess, ensure
stdin is written and ended if input is provided, remove listeners after
resolution to avoid leaks, call child.unref() as before, and ensure the Promise
rejects/returns false only on the child 'error' event rather than relying on
try/catch; then update the callers in server/cli.ts to await the Promise and
only print success messages when the awaited result is true.
---
Outside diff comments:
In `@app/helpers/feed-artwork.ts`:
- Around line 81-90: The code deletes existing artwork with
deleteFeedArtwork(feedId) then writes the new file directly to artworkPath
(ARTWORK_DIR + `${feedId}.${fileType.ext}`), risking deletion on write failure;
instead, write atomically by using Bun.file() to create a temporary file (e.g.,
artworkPath + `.tmp-${process.pid}-${Date.now()}`), write the file contents from
file.arrayBuffer() into that temp file, fs/rename the temp to artworkPath (or
use Bun's rename) only after the write succeeds, and remove the temp on error;
ensure you stop using node:fs.promises and replace write operations with
Bun.file() semantics so the original file is only replaced on successful rename.
In `@server/cli.ts`:
- Around line 50-55: The graceful-shutdown callback passed to closeWithGrace
calls server.stop(true) without awaiting it; change the callback to await
server.stop(true) (or return await server.stop(true)) so the closeWithGrace flow
waits for the server to finish closing, and wrap the await in a try/catch to log
any errors (use closeListeners/closeWithGrace and the server.stop method names
to locate the code).
---
Nitpick comments:
In @.github/workflows/publish.yml:
- Around line 11-12: Remove the redundant per-job steps that echo the
workflow-level env var FORCE_JAVASCRIPT_ACTIONS_TO_NODE24; since this variable
is already set in the workflow env, delete the echo steps in the jobs typecheck,
test, build, and publish (the steps that run commands like `echo
"FORCE_JAVASCRIPT_ACTIONS_TO_NODE24=true"`), leaving the workflow-level env
intact and ensuring no job-level duplication remains.
In `@app/helpers/feed-artwork-resolution.ts`:
- Around line 23-30: The helper currently constructs a new
Request('http://localhost') which drops HEAD/validator/range headers; update
resolveFeedArtwork to accept a caller Request (add a request: Request parameter
to resolveFeedArtwork) and forward that real request into the getFileResponse
call instead of creating a new Request, so getFileResponse(uploadedArtwork.path,
request, { cacheControl: ..., contentType: ... }) preserves conditional/file
semantics; update all callers of resolveFeedArtwork to pass their incoming
Request through.
In `@app/helpers/feed-artwork.ts`:
- Around line 24-31: Replace the blocking existsSync + fileTypeFromFile pattern
in the function that looks up artwork (using feedId and ARTWORK_DIR) with a
single async file access attempt: build artPath for each extension, call
fs.promises.readFile or fs.promises.stat wrapped in try/catch (or use
fs.promises.open) instead of existsSync, then pass the path or buffer to
fileTypeFromFile; on errors only ignore when error.code === 'ENOENT' and rethrow
other errors, and return { path: artPath, mimeType } when fileTypeFromFile
succeeds (keeping the same mimeType fallback logic). Use the existing symbols
feedId, ARTWORK_DIR, and fileTypeFromFile to locate where to change.
In `@app/helpers/ffmpeg.ts`:
- Around line 252-262: The catch block around execFileAsync can be simplified by
asserting the thrown error as the ExecFileException (or a type with stderr) and
reading error.stderr directly instead of the verbose typeof/object check; update
the catch to cast the caught variable to ExecFileException (or use "as any") and
set stderr = String((err as ExecFileException).stderr ?? err) before throwing
the new Error, referencing the existing execFileAsync call and the local stderr
variable in the ffmpeg handling code.
- Around line 289-297: The isFFmpegAvailable function can hang if ffmpeg stalls;
modify isFFmpegAvailable to enforce a timeout (e.g., 5s) when calling
execFileAsync('ffmpeg', ['-version']). Implement this by racing the
execFileAsync promise against a timeout promise that rejects after the timeout
or by using execFileAsync's timeout option if available, and ensure the catch
treats timeout as a failure (return false) and cleans up any child process if
needed.
In `@app/helpers/media.ts`:
- Around line 968-974: The current loop awaiting fs.promises.stat(p) one-by-one
on filePaths causes serial IO; replace it with a bounded-parallel stat pass that
collects results into validFileStats concurrently (e.g., map filePaths to stat
promises and run with a concurrency limit via a simple worker pool or a
p-limit-style utility), handle rejections per-file (skip or log) and push {
path: p, mtime: Number(mtimeMs) } for successful stats; ensure you still
reference filePaths, validFileStats, fs.promises.stat and mtimeMs when
implementing the concurrent/stat collection logic.
In `@app/routes/admin/api/media-stream.ts`:
- Around line 41-51: Replace the unused createLazyFile check with the direct
existence check used elsewhere: call fileExists(filePath) to verify the file
before calling serveFileWithRanges, remove the unused file variable from
createLazyFile, and keep serveFileWithRanges(filePath, context.request,
'private, max-age=3600') unchanged; this targets the file existence logic
involving createLazyFile, fileExists, serveFileWithRanges and filePath.
In `@Dockerfile`:
- Line 7: The Dockerfile RUN line installs ffmpeg with apt-get which pulls
Debian "recommended" packages; update the RUN command that contains "apt-get
install -y ffmpeg" to include the flag "--no-install-recommends" so it becomes
"apt-get install -y --no-install-recommends ffmpeg" (keeping the surrounding
"apt-get update &&" and the trailing "rm -rf /var/lib/apt/lists/*") to reduce
image size and dependency surface.
In `@server/bundling.ts`:
- Around line 170-173: Requests currently rebuild bundles on every call to
bundleEntrypoint (called with filepath and rootDir); add a production-safe
memoization layer that caches the bundle output keyed by a stable key (e.g.,
filepath + file mtime or a file content hash) and returns the cached result when
the key matches. Implement a simple in-memory Map cache in server/bundling.ts
that maps the computed key to the bundle result, invalidate/update the cache
when the mtime changes, and guard caching behind a NODE_ENV === "production"
check; apply the same caching logic to the other bundleEntrypoint invocation
referenced (lines ~206-209) so both call sites use the shared cache.
In `@server/node-server.ts`:
- Around line 62-91: Extract the duplicated close-and-await logic into a single
helper (e.g., a private async function like closeServerAndAwait or reuse
[Symbol.asyncDispose]) and have both [Symbol.asyncDispose] and stop call that
helper; the helper should run server.closeIdleConnections?.(), then return a
Promise that resolves/rejects based on server.close callback so the close
behavior is centralized and not duplicated across stop and
[Symbol.asyncDispose].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c0e8887-8d04-4dc8-9093-0ef93aeffb13
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (76)
.github/workflows/publish.yml.github/workflows/release.yml.gitignore.oxlintrc.jsonAGENTS.mdDockerfileREADME.mdapp/cache/cache.tsapp/client/admin/edit-route-paths.test.tsapp/config/env.tsapp/db/bun-sqlite-adapter-compat.tsapp/db/feed-analytics-events.test.tsapp/db/feed-analytics-events.tsapp/db/feeds.test.tsapp/db/index.tsapp/db/migrations.analytics.test.tsapp/db/migrations.tsapp/db/sqlite.tsapp/db/test-database.tsapp/helpers/analytics-request.test.tsapp/helpers/analytics-window.test.tsapp/helpers/bundle-version.tsapp/helpers/decode-path-param.test.tsapp/helpers/exec.tsapp/helpers/feed-artwork-resolution.tsapp/helpers/feed-artwork.tsapp/helpers/feed-list-sort.test.tsapp/helpers/ffmpeg.tsapp/helpers/format.test.tsapp/helpers/media-list-sort.test.tsapp/helpers/media.test.tsapp/helpers/media.tsapp/helpers/node-file.tsapp/helpers/origin.test.tsapp/helpers/range-request.tsapp/helpers/rate-limiter.test.tsapp/helpers/rss.test.tsapp/helpers/version.test.tsapp/helpers/version.tsapp/mcp/auth.test.tsapp/mcp/resources.tsapp/middleware/rate-limit.test.tsapp/oauth/client-metadata.test.tsapp/oauth/oauth.test.tsapp/oauth/register.test.tsapp/router.tsxapp/routes/admin/api/artwork.tsapp/routes/admin/api/feeds-id-analytics.test.tsapp/routes/admin/api/media-analytics-path.test.tsapp/routes/admin/api/media-analytics.$path.tsapp/routes/admin/api/media-stream.tsapp/routes/admin/api/media-upload.tsapp/routes/admin/api/media.test.tsapp/routes/admin/api/path-decoding.test.tsapp/routes/art-route.test.tsapp/routes/art.tsapp/routes/feed-route-analytics.test.tsapp/routes/mcp/oauth-protected-resource-route.test.tsapp/routes/mcp/widget-route.test.tsapp/routes/media-route-analytics.test.tsapp/routes/media-route.test.tsapp/routes/media.tsapp/routes/oauth/token.tsindex.tspackage.jsonplaywright-config.tsscripts/release.tsserver/bundling.tsserver/cli.tsserver/node-server.tsserver/platform-cli.tsserver/register-hooks.mjstest/bun-test-compat.tstest/setup.tstest/test-helpers.tstsconfig.json
| - `Bun.redis` for Redis. Don't use `ioredis`. | ||
| - `Bun.sql` for Postgres. Don't use `pg` or `postgres.js`. | ||
| - Use the Node HTTP stack plus `remix/node-fetch-server` for the server entrypoint. | ||
| - Use `node:sqlite` for SQLite. |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
node:sqlite module Node.js 24 experimental status
💡 Result:
In Node.js 24, the node:sqlite module is experimental with stability level 1.1 (active development). It does not require the --experimental-sqlite flag, as the module was unflagged in November 2024 (PR #55890, landed before Node.js 24.0.0 release in May 2025). The flag was required initially from v22.5.0 until v23.4.0/v22.13.0. Official docs for v24.x (e.g., latest-v24.x) confirm availability without mention of the flag and list stability as 1.1 - Active development. Not recommended for production due to experimental status; breaking changes possible without semver guarantees.
Citations:
- 1: https://nodejs.org/docs/latest-v24.x/api/sqlite.html
- 2: https://nodejs.org/api/documentation.html
- 3: https://nodejs.org/docs/latest-v24.x/api/documentation.html
- 4: doc,lib,src,test: unflag sqlite module nodejs/node#55890
- 5: https://nodejs.org/api/sqlite.html
- 6: stabilization of node:sqlite module nodejs/node#57445
- 7: https://beta.docs.nodejs.org/sqlite.html
- 8: https://nodejs.org/download/nightly/v24.0.0-nightly2024121426d0559f14/docs/api/sqlite.html
node:sqlite is available in Node.js 24 but remains experimental.
The node:sqlite module is available in Node.js 24 without requiring the --experimental-sqlite flag (unflagged since November 2024). However, it retains stability level 1.1 (active development) and is not recommended for production use, as breaking changes are possible without semver guarantees. Either document this experimental status in the guide, or verify that tests adequately cover SQLite operations and accept the experimental nature as acceptable for this project.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` at line 348, The doc line "Use `node:sqlite` for SQLite." should
be updated to warn that `node:sqlite` (available unflagged in Node.js 24) is
still experimental (stability 1.1) and not recommended for production; either
add a short note in AGENTS.md stating its experimental status, Node.js version
requirement, and potential for breaking changes without semver guarantees, or
alternatively add/point to test coverage that validates SQLite operations
(mention tests that exercise SQLite behavior) to accept its use—update the
sentence to include this guidance and a brief recommendation for production
users.
| function normalizeRunResult(result: StatementResultingChanges) { | ||
| return { | ||
| changes: | ||
| typeof result.changes === 'bigint' | ||
| ? Number(result.changes) | ||
| : result.changes, | ||
| lastInsertRowid: | ||
| typeof result.lastInsertRowid === 'bigint' | ||
| ? Number(result.lastInsertRowid) | ||
| : result.lastInsertRowid, | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential precision loss for large rowids.
Converting bigint to number can lose precision for values exceeding Number.MAX_SAFE_INTEGER (9,007,199,254,740,991). While unlikely in typical usage, consider keeping as bigint or adding a range check if this database may have very high row counts.
🛡️ Optional: Add overflow check
function normalizeRunResult(result: StatementResultingChanges) {
+ if (
+ typeof result.lastInsertRowid === 'bigint' &&
+ result.lastInsertRowid > BigInt(Number.MAX_SAFE_INTEGER)
+ ) {
+ console.warn('lastInsertRowid exceeds MAX_SAFE_INTEGER, precision may be lost')
+ }
return {
changes:
typeof result.changes === 'bigint'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/db/sqlite.ts` around lines 29 - 40, normalizeRunResult currently converts
bigint fields in StatementResultingChanges to number which can lose precision
for values > Number.MAX_SAFE_INTEGER; change normalizeRunResult to avoid blind
Number(...) conversion by either (A) preserving bigint values when
result.changes or result.lastInsertRowid are bigints, (B) converting bigints to
string when they exceed Number.MAX_SAFE_INTEGER, or (C) perform a safe-range
check (compare to Number.MAX_SAFE_INTEGER) and only Number(...) when safe,
otherwise return the bigint/string; update the returned shape/type annotations
accordingly so callers of normalizeRunResult and any consumers of
changes/lastInsertRowid handle bigint or string values instead of assuming
number.
| @@ -1,3 +1,4 @@ | |||
| import fs from 'node:fs' | |||
There was a problem hiding this comment.
Update runtime policy docs to match the Node migration.
Line [1] switches this helper to Node filesystem APIs, but the current repository runtime rules still mandate Bun-first usage. Please update the project guidance (AGENTS/coding-guidelines) in this PR so future changes/reviews don’t conflict with the new Node 24 direction.
As per coding guidelines: "Use bun <file> instead of node <file> or ts-node <file>" and "Prefer Bun.file over node:fs's readFile/writeFile for file operations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/helpers/feed-artwork.ts` at line 1, The project docs still mandate
Bun-first APIs but this PR switches app/helpers/feed-artwork.ts to use Node's
"node:fs" import; update the AGENTS/coding-guidelines doc to reflect the Node 24
migration by removing or altering the "Use `bun <file>`" and "Prefer `Bun.file`
over `node:fs`" rules, add a clear statement that Node 24 and node:fs APIs are
now preferred, and give guidance on when Bun usage is acceptable (if at all);
ensure the guideline references the specific change (app/helpers/feed-artwork.ts
using node:fs) so future reviewers accept Node filesystem usage.
| const allowedHosts = process.env.ALLOWED_HOSTS?.split(',').map((h) => | ||
| h.trim(), | ||
| ) |
There was a problem hiding this comment.
Filter empty allowlist entries to prevent accidental deny-all.
On Line 183, split(',').map(trim) can leave empty tokens. If ALLOWED_HOSTS is empty or has trailing commas, host validation on Line 189 can reject every request unintentionally.
🔧 Proposed fix
- const allowedHosts = process.env.ALLOWED_HOSTS?.split(',').map((h) =>
- h.trim(),
- )
+ const allowedHosts = process.env.ALLOWED_HOSTS
+ ?.split(',')
+ .map((h) => h.trim())
+ .filter(Boolean)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const allowedHosts = process.env.ALLOWED_HOSTS?.split(',').map((h) => | |
| h.trim(), | |
| ) | |
| const allowedHosts = process.env.ALLOWED_HOSTS | |
| ?.split(',') | |
| .map((h) => h.trim()) | |
| .filter(Boolean) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/routes/oauth/token.ts` around lines 183 - 185, The allowedHosts parsing
can produce empty tokens and inadvertently create an empty allowlist; update the
logic that builds allowedHosts to split, trim, and filter out empty strings
(e.g., .split(',').map(h=>h.trim()).filter(Boolean)), and if the resulting array
is empty set allowedHosts to undefined/null so the later host validation (the
host-check using allowedHosts) won’t treat an empty list as a deny-all; modify
the variable creation where allowedHosts is computed and ensure the downstream
host validation uses the adjusted allowedHosts value.
| import { startNodeServer } from './server/node-server.ts' | ||
|
|
||
| const env = getEnv() | ||
| const rootDir = new URL('.', import.meta.url).pathname |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js ESM, should filesystem paths derived from import.meta.urlusefileURLToPath(new URL('.', import.meta.url))instead ofnew URL('.', import.meta.url).pathname?
💡 Result:
Yes, in Node.js ESM, for filesystem paths derived from import.meta.url to get the current module's directory (equivalent to dirname), you should use fileURLToPath(new URL('.', import.meta.url)) instead of new URL('.', import.meta.url).pathname. The .pathname property returns a URL-encoded path string that starts with a leading slash on Windows (e.g., /C:/path/), which is invalid for Node.js filesystem APIs like fs and path modules. It may also leave percent-encoding intact for special characters (e.g., spaces as %20), causing issues with path operations. fileURLToPath converts the file:// URL to a proper, decoded, platform-native filesystem path (e.g., C:\path on Windows with backslashes, /path on Unix). Examples: import { fileURLToPath } from 'node:url'; import { dirname } from 'node:path'; const dirname = dirname(fileURLToPath(new URL('.', import.meta.url))); Note: Since Node.js v20.11.0 / v21.2.0 (stable in v22.16.0+), prefer the simpler import.meta.dirname for the same result. Many Node APIs (e.g., fs.readFileSync(new URL('./file.txt', import.meta.url))) accept URL objects directly, avoiding path strings altogether.
Citations:
- 1: https://nodejs.org/api/esm.html
- 2: new URL('.', import.meta.url).pathname produces erroneous path on Windows 10 nodejs/node#37845
- 3: https://nodejs.org/api/url.html
- 4: https://www.thenodebook.com/modules/import-meta-caching
- 5: esm: add import.meta.dirname and import.meta.filename nodejs/node#48740
🏁 Script executed:
# Check for package.json and bunfig.toml to determine runtime
fd -e json -e toml | grep -E '(package\.json|bunfig\.toml|tsconfig)' | head -20Repository: kentcdodds/mediarss
Length of output: 101
🏁 Script executed:
# Search for existing import.meta usage patterns in the codebase
rg "import\.meta\.(dirname|url)" -A 2 -B 2Repository: kentcdodds/mediarss
Length of output: 1365
🏁 Script executed:
# Check if the project has Node.js version constraints
cat package.json 2>/dev/null | grep -A 5 '"engines"' || echo "No engines field"Repository: kentcdodds/mediarss
Length of output: 160
Use import.meta.dirname instead of .pathname for the filesystem path.
Line 18 uses .pathname which returns a URL path, not a filesystem path. This breaks on Windows (/C:/...) and preserves percent-encoded characters (e.g., %20 for spaces), causing createBundlingRoutes to resolve the wrong files.
The project already uses import.meta.dirname consistently in other files (app/helpers/version.ts, app/helpers/bundle-version.ts), which is the correct pattern for Node.js ≥20.11.0 and available in your required Node.js ≥24.12.0.
💡 Suggested fix
import { startNodeServer } from './server/node-server.ts'
const env = getEnv()
-const rootDir = new URL('.', import.meta.url).pathname
+const rootDir = import.meta.dirname📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { startNodeServer } from './server/node-server.ts' | |
| const env = getEnv() | |
| const rootDir = new URL('.', import.meta.url).pathname | |
| import { startNodeServer } from './server/node-server.ts' | |
| const env = getEnv() | |
| const rootDir = import.meta.dirname |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.ts` around lines 15 - 18, Replace the filesystem URL construction using
new URL('.', import.meta.url).pathname with import.meta.dirname so rootDir holds
a proper filesystem path (not a percent-encoded or URL-style path); update the
assignment to rootDir (used by createBundlingRoutes and any consumers) to use
import.meta.dirname consistent with other files (e.g., app/helpers/version.ts)
to avoid Windows and encoding issues.
| #!/usr/bin/env bun | ||
| import { $ } from 'bun' | ||
| #!/usr/bin/env node | ||
| import { readFile, writeFile } from 'node:fs/promises' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/release.ts | head -60Repository: kentcdodds/mediarss
Length of output: 2287
🏁 Script executed:
# Verify the URL.pathname issue and the fileURLToPath solution
# Check if this is indeed a cross-platform issue
python3 << 'EOF'
# Demonstrate the URL.pathname issue on Windows paths
from urllib.parse import urlparse, unquote
import os
# Simulate what URL.pathname returns on different systems
windows_file_url = "file:///C:/Users/name/project/package.json"
posix_file_url = "file:///home/name/project/package.json"
# This simulates what .pathname returns
parsed_windows = urlparse(windows_file_url)
parsed_posix = urlparse(posix_file_url)
print("Windows URL pathname:", repr(parsed_windows.path))
print("POSIX URL pathname:", repr(parsed_posix.path))
print()
print("Analysis:")
print("- Windows URL.pathname includes leading slash and colons: starts with /C:")
print("- POSIX URL.pathname is correct: /home/name/project/package.json")
print("- fileURLToPath() would convert /C:/Users/name/project/package.json to C:\\Users\\name\\project\\package.json")
EOFRepository: kentcdodds/mediarss
Length of output: 432
🏁 Script executed:
# Verify the suggested fix is correct by checking Node.js documentation pattern
# Check if fileURLToPath is the standard approach
rg -A 5 "fileURLToPath" . --type ts --type js 2>/dev/null | head -30Repository: kentcdodds/mediarss
Length of output: 45
🏁 Script executed:
# Verify there are no other uses of URL.pathname in the codebase that might have similar issues
rg "\.pathname" . --type ts --type js 2>/dev/nullRepository: kentcdodds/mediarss
Length of output: 3275
Use fileURLToPath() for packageJsonPath.
URL.pathname fails on Windows by preserving the leading slash and colons in the path (e.g., /C:/Users/...), causing file operations to fail. Use fileURLToPath() to convert the file URL to a proper platform-specific path.
Suggested fix
-import { readFile, writeFile } from 'node:fs/promises'
+import { readFile, writeFile } from 'node:fs/promises'
+import { fileURLToPath } from 'node:url'
@@
- const packageJsonPath = new URL('../package.json', import.meta.url).pathname
+ const packageJsonPath = fileURLToPath(
+ new URL('../package.json', import.meta.url),
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/release.ts` at line 2, The packageJsonPath is being built from a file
URL using URL.pathname which breaks on Windows; change the code to import
fileURLToPath from 'url' and convert the file URL to a platform path (use
fileURLToPath(packageJsonUrl)) when assigning packageJsonPath (where
packageJsonUrl and packageJsonPath are defined) so subsequent readFile/writeFile
calls use a correct OS path.
| function spawnAndForget( | ||
| command: string, | ||
| args: string[], | ||
| input?: string, | ||
| ): boolean { | ||
| try { | ||
| const child = spawn(command, args, { | ||
| detached: true, | ||
| stdio: input ? ['pipe', 'ignore', 'ignore'] : 'ignore', | ||
| }) | ||
| if (input) { | ||
| child.stdin?.end(input) | ||
| } | ||
| child.unref() | ||
| return true | ||
| } catch { | ||
| return false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and view its current state
fd -t f "platform-cli.ts" server/Repository: kentcdodds/mediarss
Length of output: 85
🏁 Script executed:
# Check the entire file to understand context
cat -n server/platform-cli.ts 2>/dev/null || echo "File not found"Repository: kentcdodds/mediarss
Length of output: 1289
🏁 Script executed:
# Check how the CLI uses these spawn functions
fd -t f "cli.ts" server/ | head -5Repository: kentcdodds/mediarss
Length of output: 99
🏁 Script executed:
# Search for usage of spawnAndForget in the codebase
rg "spawnAndForget" server/ -A 2 -B 2Repository: kentcdodds/mediarss
Length of output: 1290
🏁 Script executed:
# Check how server/cli.ts uses these functions
cat -n server/cli.tsRepository: kentcdodds/mediarss
Length of output: 4015
🏁 Script executed:
# Search for patterns where these functions are called and what happens after
rg "openInBrowser|copyToClipboard" server/cli.ts -A 3 -B 1Repository: kentcdodds/mediarss
Length of output: 479
🌐 Web query:
Node.js child_process spawn missing executable error event async
💡 Result:
The "Error: spawn ENOENT" in Node.js child_process.spawn occurs when the executable cannot be found. This is an asynchronous error emitted via the 'error' event on the returned ChildProcess instance. To handle it properly: 1. Listen for the 'error' event immediately after spawn: const { spawn } = require('node:child_process'); const child = spawn('nonexistent-command', []); child.on('error', (err) => { if (err.code === 'ENOENT') { console.error(Executable not found: ${err.path || 'unknown'}); } else { console.error('Spawn error:', err.message); } }); Without this listener, the error becomes unhandled and crashes the process with "Unhandled 'error' event". Common causes and fixes: - Command not in PATH: Use full absolute path, e.g., spawn('/usr/bin/ls', []) - Invalid cwd in options: Ensure options.cwd exists - Windows specifics: Use '.cmd' or '.bat' extension for batch files, e.g., process.platform === 'win32' ? 'command.cmd' : 'command' - Empty PATH in options.env: Node uses options.env.PATH for lookup; inherit or set properly - Debug with NODE_DEBUG=child_process or wrap spawn to log calls The spawn call itself is async and non-blocking (though may briefly block event loop on large heaps), but error emission is async via EventEmitter. 'spawn' event emits only on success; 'error' on failure like ENOENT.
Citations:
- 1: https://nodejs.org/api/child_process.html
- 2: https://nodejs.org/docs/latest/api/child_process.html
- 3: https://nodejs.org/docs/latest-v18.x/api/child_process.html
- 4: https://stackoverflow.com/questions/34208614/how-to-catch-an-enoent-with-nodejs-child-process-spawn
- 5: https://stackoverflow.com/questions/27688804/how-do-i-debug-error-spawn-enoent-on-node-js
Add error handling to prevent unhandled 'error' events from crashing the CLI when executables are missing.
Node's spawn() reports missing executables like xdg-open, xclip, pbcopy, or clip asynchronously via the 'error' event on the child process—not through synchronous exceptions. Without an error listener, these failures will crash the CLI. Additionally, callers in server/cli.ts ignore the boolean return value and unconditionally print success messages (lines 97–99), creating false positive feedback.
Convert to a Promise-based API that listens for both 'error' and 'spawn' events:
Suggested direction
function spawnAndForget(
command: string,
args: string[],
input?: string,
-): boolean {
- try {
- const child = spawn(command, args, {
- detached: true,
- stdio: input ? ['pipe', 'ignore', 'ignore'] : 'ignore',
- })
- if (input) {
- child.stdin?.end(input)
- }
- child.unref()
- return true
- } catch {
- return false
- }
+): Promise<boolean> {
+ return new Promise((resolve) => {
+ try {
+ const child = spawn(command, args, {
+ detached: true,
+ stdio: input ? ['pipe', 'ignore', 'ignore'] : 'ignore',
+ })
+ child.once('error', () => resolve(false))
+ child.once('spawn', () => {
+ if (input) child.stdin?.end(input)
+ child.unref()
+ resolve(true)
+ })
+ } catch {
+ resolve(false)
+ }
+ })
}Then await the result in server/cli.ts before logging success.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function spawnAndForget( | |
| command: string, | |
| args: string[], | |
| input?: string, | |
| ): boolean { | |
| try { | |
| const child = spawn(command, args, { | |
| detached: true, | |
| stdio: input ? ['pipe', 'ignore', 'ignore'] : 'ignore', | |
| }) | |
| if (input) { | |
| child.stdin?.end(input) | |
| } | |
| child.unref() | |
| return true | |
| } catch { | |
| return false | |
| } | |
| function spawnAndForget( | |
| command: string, | |
| args: string[], | |
| input?: string, | |
| ): Promise<boolean> { | |
| return new Promise((resolve) => { | |
| try { | |
| const child = spawn(command, args, { | |
| detached: true, | |
| stdio: input ? ['pipe', 'ignore', 'ignore'] : 'ignore', | |
| }) | |
| child.once('error', () => resolve(false)) | |
| child.once('spawn', () => { | |
| if (input) child.stdin?.end(input) | |
| child.unref() | |
| resolve(true) | |
| }) | |
| } catch { | |
| resolve(false) | |
| } | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/platform-cli.ts` around lines 3 - 20, The spawnAndForget function
currently swallows async child-process 'error' events and returns synchronously;
change it to an async Promise-based API (e.g., export or rename spawnAndForget
to return Promise<boolean>) that attaches both an 'error' listener (resolve
false) and a 'spawn' or 'close' listener (resolve true) on the spawned
ChildProcess, ensure stdin is written and ended if input is provided, remove
listeners after resolution to avoid leaks, call child.unref() as before, and
ensure the Promise rejects/returns false only on the child 'error' event rather
than relying on try/catch; then update the callers in server/cli.ts to await the
Promise and only print success messages when the awaited result is true.
|
@cursor, please address the PR CI failure: 31s (node:20) ExperimentalWarning: SQLite is an experimental feature and might change at any time
Running migration 1: initial_schema |
This comment has been minimized.
This comment has been minimized.
|
@cursoragent please take a look at the PR feedback here and address any valid issues. Also, the Docker build failed again with the following error. Please fix: Run # Start the container
Running migration 1: initial_schema If you're not sure what to do to fix this, then just add necessary logs during the test so that we can debug it. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@cursoragent I notice we still have files that reference Bun. In particular, our SQLite Bun adapter is there. Could you check the Remix documentation and see whether there is a more appropriate module that we should use in a Node context with the Remix database utilities? Again... |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
server/register-hooks.mjs (1)
38-39: Gate inline sourcemaps by environment to match bundling behavior.The
sourcemap: 'inline'setting on line 38 is hard-coded, whileserver/bundling.tsalready gates sourcemaps conditionally (sourcemap: isProduction ? false : 'inline'). Aligning the runtime loader with the bundle configuration prevents unnecessary overhead and avoids a runtime-vs-bundle split if this hook is reused outside local development.♻️ Proposed fix
+const isProduction = process.env.NODE_ENV === 'production' + registerHooks({ resolve(specifier, context, nextResolve) { if (specifier === 'bun:test') { return { @@ const transformed = transformSync(source, { format: 'esm', jsx: 'automatic', jsxImportSource: 'remix/component', loader: 'tsx', sourcefile: filename, - sourcemap: 'inline', + sourcemap: isProduction ? false : 'inline', target: 'esnext', })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/register-hooks.mjs` around lines 38 - 39, The hard-coded sourcemap: 'inline' in register-hooks.mjs should be gated by the runtime environment to match server/bundling.ts; compute the same production flag (e.g., isProduction derived from NODE_ENV or reuse the same helper) and set sourcemap to isProduction ? false : 'inline' instead of the literal 'inline' so runtime loader behavior matches bundle configuration and avoids runtime-vs-bundle divergence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/db/feed-analytics-events.ts`:
- Line 3: Replace the incorrect external type import with the local SQLite
wrapper type: change the import of "Database" from 'better-sqlite3' to the
Database export from the project's SQLite wrapper module so the file types match
the .query(...) usage; update the import at the top (the current "import type
Database from 'better-sqlite3'") to import the Database type exported by the
local wrapper (the module that defines Database and .query) so TypeScript
resolves correctly and the TS7016 error is resolved.
In `@server/node-server.ts`:
- Around line 27-40: The async http.createServer listener (the function creating
request, client, calling handler, and sendResponse) must be wrapped in a
try/catch to prevent unhandled promise rejections: catch any error thrown by
createRequest, handler, or sendResponse, log the error, and send a proper error
response (e.g., a 500 status and a minimal body) using the same res object and
ensure the response is ended/headers flushed so the socket is not left hanging;
update the listener surrounding the references to createRequest, handler, and
sendResponse (and the server variable) to implement this behavior and always
close or end the response on error.
In `@server/register-hooks.mjs`:
- Around line 32-40: The TSX runtime transform in server/register-hooks.mjs uses
transformSync(...) with loader:'tsx' but omits verbatimModuleSyntax, causing
module semantics to differ from bundling.ts and tsconfig.json; update the
transform options object passed to transformSync (the same one that sets format,
jsx, jsxImportSource, loader, sourcefile, sourcemap, target) to include
verbatimModuleSyntax: true so the runtime transform preserves import/exports the
same way the bundler does.
---
Nitpick comments:
In `@server/register-hooks.mjs`:
- Around line 38-39: The hard-coded sourcemap: 'inline' in register-hooks.mjs
should be gated by the runtime environment to match server/bundling.ts; compute
the same production flag (e.g., isProduction derived from NODE_ENV or reuse the
same helper) and set sourcemap to isProduction ? false : 'inline' instead of the
literal 'inline' so runtime loader behavior matches bundle configuration and
avoids runtime-vs-bundle divergence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d63a946b-8429-42de-a220-3597084ce781
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
app/db/bun-data-table-sql-compiler.tsapp/db/bun-sqlite-adapter-compat.tsapp/db/data-table.tsapp/db/feed-analytics-events.tsapp/db/index.tsapp/db/migrations.tsapp/db/sqlite.tsindex.tspackage.jsonserver/node-server.tsserver/register-hooks.mjs
💤 Files with no reviewable changes (2)
- app/db/bun-sqlite-adapter-compat.ts
- app/db/bun-data-table-sql-compiler.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/db/index.ts
- app/db/migrations.ts
- package.json
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/routes/admin/api/media-upload.ts (1)
248-260:⚠️ Potential issue | 🟠 MajorComment is now misleading —
writeBlobToFileloads the entire file into memory.The comment claims this "avoids loading entire file into memory," but the helper function does exactly that:
// from node-file.ts await fs.promises.writeFile(path, Buffer.from(await blob.arrayBuffer()))
blob.arrayBuffer()materializes the full blob in memory. With a 10GB file size limit (line 189), this could cause OOM failures on large uploads.Consider either:
- Updating the comment to reflect the actual behavior, or
- Using Node streams for true streaming writes (e.g.,
Readable.fromWeb(file.stream()).pipe(createWriteStream(tempPath)))Option 1: Fix the misleading comment
- // Stream the file to disk first (avoids loading entire file into memory) + // Write the file to disk (note: loads file into memory via arrayBuffer)Option 2: Use Node streams for true streaming (preferred for large files)
+import { Readable } from 'node:stream' +import { createWriteStream } from 'node:fs' +import { pipeline } from 'node:stream/promises'Then in the helper or inline:
async function streamBlobToFile(path: string, blob: Blob): Promise<void> { const nodeReadable = Readable.fromWeb(blob.stream() as any) await pipeline(nodeReadable, createWriteStream(path)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/admin/api/media-upload.ts` around lines 248 - 260, The comment above the writeBlobToFile call is misleading because writeBlobToFile currently uses blob.arrayBuffer() (which loads the entire file into memory) and risks OOM for large uploads; either update the comment to reflect that behavior or (preferred) replace writeBlobToFile with a streaming implementation: add a new helper streamBlobToFile (or modify writeBlobToFile) that converts the Blob's web stream to a Node readable (Readable.fromWeb(file.stream())), then use pipeline with fs.createWriteStream(tempPath) to stream to disk without buffering the whole file; reference writeBlobToFile, blob.arrayBuffer(), file.stream(), Readable.fromWeb, pipeline, createWriteStream, and tempPath when making the change.
🧹 Nitpick comments (3)
app/db/migrations.ts (1)
2-2: Use inline type specifier per linter suggestion.Static analysis flags this import. Using inline type specifier is more explicit and aligns with
verbatimModuleSyntax.🔧 Proposed fix
-import type { Database } from './sqlite.ts' +import { type Database } from './sqlite.ts'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/db/migrations.ts` at line 2, Change the import to use an inline type specifier so the linter/verbatimModuleSyntax is satisfied: replace the current import of Database from './sqlite.ts' with an inline type import (i.e., import { type Database } from './sqlite.ts') so that the symbol Database is explicitly imported as a type in the module where Database is referenced.app/db/sqlite.ts (2)
75-79: Consider documenting the timeout or making it configurable.The 5-second timeout is hardcoded. For most use cases this is fine, but if long migrations or bulk operations are anticipated, this could cause unexpected failures.
💡 Optional: Accept timeout as constructor option
- constructor(path: string) { + constructor(path: string, options?: { timeout?: number }) { this.#database = new DatabaseSync(path, { - timeout: 5_000, + timeout: options?.timeout ?? 5_000, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/db/sqlite.ts` around lines 75 - 79, The constructor for the SQLite wrapper hardcodes a 5_000ms timeout when creating DatabaseSync, which should be configurable and documented; update the class constructor (constructor) to accept an optional options param (e.g., { timeout?: number } with default 5000), pass that timeout into the DatabaseSync instantiation instead of the hardcoded value, and add a brief JSDoc comment on the constructor explaining the timeout's purpose and default so callers can adjust it for long migrations or bulk operations (refer to DatabaseSync and the constructor in sqlite.ts).
97-102: Unused generic parameter_TParams.The
_TParamsgeneric onprepareis declared but never used. If it's intentional for API parity withbun:sqlite, consider documenting that; otherwise, it can be removed.🔧 Remove unused generic or use it
prepare< TRow = Record<string, unknown>, - _TParams extends DatabaseParameters = [], >(sql: string): PreparedStatement<TRow> { return new PreparedStatement<TRow>(this.#database.prepare(sql)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/db/sqlite.ts` around lines 97 - 102, The prepare method declares an unused generic parameter _TParams which should be removed or used; update the prepare<TRow = Record<string, unknown>>(sql: string): PreparedStatement<TRow> signature to drop the unused _TParams generic, and if API parity with bun:sqlite is required, instead add a short comment on the prepare method documenting that a second generic was intentionally omitted for simplicity; ensure references to PreparedStatement<TRow> and the prepare method remain correct after the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/register-hooks.mjs`:
- Line 39: The load hook currently only matches '.tsx' files (the condition
url.endsWith('.tsx')) so '.ts' files are not being transformed; update the
condition in the load hook to include both '.ts' and '.tsx' (e.g., test for
/\.tsx?$/ or check url.endsWith('.ts') || url.endsWith('.tsx')) so TypeScript
type syntax is stripped/compiled the same way as .tsx files; locate the load
hook where url.endsWith('.tsx') appears and replace the condition accordingly.
---
Outside diff comments:
In `@app/routes/admin/api/media-upload.ts`:
- Around line 248-260: The comment above the writeBlobToFile call is misleading
because writeBlobToFile currently uses blob.arrayBuffer() (which loads the
entire file into memory) and risks OOM for large uploads; either update the
comment to reflect that behavior or (preferred) replace writeBlobToFile with a
streaming implementation: add a new helper streamBlobToFile (or modify
writeBlobToFile) that converts the Blob's web stream to a Node readable
(Readable.fromWeb(file.stream())), then use pipeline with
fs.createWriteStream(tempPath) to stream to disk without buffering the whole
file; reference writeBlobToFile, blob.arrayBuffer(), file.stream(),
Readable.fromWeb, pipeline, createWriteStream, and tempPath when making the
change.
---
Nitpick comments:
In `@app/db/migrations.ts`:
- Line 2: Change the import to use an inline type specifier so the
linter/verbatimModuleSyntax is satisfied: replace the current import of Database
from './sqlite.ts' with an inline type import (i.e., import { type Database }
from './sqlite.ts') so that the symbol Database is explicitly imported as a type
in the module where Database is referenced.
In `@app/db/sqlite.ts`:
- Around line 75-79: The constructor for the SQLite wrapper hardcodes a 5_000ms
timeout when creating DatabaseSync, which should be configurable and documented;
update the class constructor (constructor) to accept an optional options param
(e.g., { timeout?: number } with default 5000), pass that timeout into the
DatabaseSync instantiation instead of the hardcoded value, and add a brief JSDoc
comment on the constructor explaining the timeout's purpose and default so
callers can adjust it for long migrations or bulk operations (refer to
DatabaseSync and the constructor in sqlite.ts).
- Around line 97-102: The prepare method declares an unused generic parameter
_TParams which should be removed or used; update the prepare<TRow =
Record<string, unknown>>(sql: string): PreparedStatement<TRow> signature to drop
the unused _TParams generic, and if API parity with bun:sqlite is required,
instead add a short comment on the prepare method documenting that a second
generic was intentionally omitted for simplicity; ensure references to
PreparedStatement<TRow> and the prepare method remain correct after the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd44688c-eac0-432c-af58-e0f3327736b5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
.github/workflows/publish.ymlapp/db/bun-sqlite-adapter-compat.tsapp/db/feed-analytics-events.tsapp/db/index.tsapp/db/migrations.tsapp/db/sqlite.tsapp/helpers/node-file.tsapp/routes/admin/api/media-upload.tspackage.jsonserver/node-server.tsserver/register-hooks.mjs
✅ Files skipped from review due to trivial changes (1)
- app/db/bun-sqlite-adapter-compat.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- app/db/index.ts
- package.json
- app/db/feed-analytics-events.ts
- .github/workflows/publish.yml
- server/node-server.ts
- app/helpers/node-file.ts
|
@cursoragent can you check the docker build failure in CI of this PR and fix that? |
This comment has been minimized.
This comment has been minimized.
| mediaRequests: 1, | ||
| }) | ||
| expect(data.byToken[0]?.createdAt).not.toBeNull() | ||
| expect(data.byToken[0]?.createdAt).toBeNull() |
There was a problem hiding this comment.
Test expectations changed to hide analytics behavioral regression
Medium Severity
The test now expects label: 'Deleted token' and createdAt to be null after feed deletion, whereas previously it expected the original label 'Soon deleted token' and a non-null createdAt. This indicates that deleting a directory feed now also cascades to delete associated tokens from the database, so analytics can no longer display the original token metadata for deleted feeds. This is a functional regression in the analytics feature caused by the SQLite runtime migration.
|
@cursoragent please check on feedback on this PR again. For valid feedback, fix it, for invalid feedback tell me why it's invalid. |
This comment has been minimized.
This comment has been minimized.
|
@cursoragent please address any valid feedback on this PR |
|
Bugbot Autofix prepared fixes for both issues found in the latest run.
Preview (8d02f088e5)diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml
--- a/.github/workflows/publish.yml
+++ b/.github/workflows/publish.yml
@@ -22,61 +22,70 @@
runs-on: ubuntu-22.04
steps:
- name: ⬇️ Checkout repo
- uses: actions/checkout@v4
+ uses: actions/checkout@v6
- - name: 📦 Setup Bun
- uses: oven-sh/setup-bun@v2
+ - name: 📦 Setup Node
+ uses: actions/setup-node@v6
+ with:
+ node-version: 24
+ cache: npm
- name: 📥 Install dependencies
- run: bun install --frozen-lockfile
+ run: npm ci
- name: 🔬 Lint
- run: bun run check:ci
+ run: npm run lint:check
typecheck:
name: ʦ TypeCheck
runs-on: ubuntu-22.04
steps:
- name: ⬇️ Checkout repo
- uses: actions/checkout@v4
+ uses: actions/checkout@v6
- - name: 📦 Setup Bun
- uses: oven-sh/setup-bun@v2
+ - name: 📦 Setup Node
+ uses: actions/setup-node@v6
+ with:
+ node-version: 24
+ cache: npm
- name: 📥 Install dependencies
- run: bun install --frozen-lockfile
+ run: npm ci
- name: 🔎 Type check
- run: bun run typecheck
+ run: npm run typecheck
test:
name: 🧪 Test
runs-on: ubuntu-22.04
steps:
- name: ⬇️ Checkout repo
- uses: actions/checkout@v4
+ uses: actions/checkout@v6
- - name: 📦 Setup Bun
- uses: oven-sh/setup-bun@v2
+ - name: 📦 Setup Node
+ uses: actions/setup-node@v6
+ with:
+ node-version: 24
+ cache: npm
- name: 📥 Install dependencies
- run: bun install --frozen-lockfile
+ run: npm ci
- name: 🧪 Run tests
- run: bun test
+ run: npm test
build:
name: 🐳 Build
runs-on: ubuntu-22.04
steps:
- name: ⬇️ Checkout repo
- uses: actions/checkout@v4
+ uses: actions/checkout@v6
- name: 🐳 Set up Docker Buildx
- uses: docker/setup-buildx-action@v3
+ uses: docker/setup-buildx-action@v4
- name: 🏗️ Build Docker image
- uses: docker/build-push-action@v5
+ uses: docker/build-push-action@v6
with:
context: .
file: ./Dockerfile
@@ -94,6 +103,12 @@
# Wait for the container to be ready (max 30 seconds)
for i in {1..30}; do
+ if [ "$(docker inspect -f '{{.State.Running}}' smoke-test 2>/dev/null)" != "true" ]; then
+ echo "❌ Container exited before health check passed!"
+ docker logs smoke-test || true
+ docker rm -f smoke-test || true
+ exit 1
+ fi
if curl -sf http://localhost:22050/admin/health > /dev/null 2>&1; then
echo "✅ Health check passed!"
curl -s http://localhost:22050/admin/health | jq .
@@ -118,20 +133,20 @@
if: ${{ github.event_name == 'push' }}
steps:
- name: ⬇️ Checkout repo
- uses: actions/checkout@v4
+ uses: actions/checkout@v6
- name: 🐳 Set up Docker Buildx
- uses: docker/setup-buildx-action@v3
+ uses: docker/setup-buildx-action@v4
- name: 🔑 Login to Docker Hub
- uses: docker/login-action@v3
+ uses: docker/login-action@v4
with:
username: ${{ secrets.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_TOKEN }}
- name: 📝 Generate Docker metadata
id: meta
- uses: docker/metadata-action@v5
+ uses: docker/metadata-action@v6
with:
images: ${{ secrets.DOCKERHUB_USERNAME }}/mediarss
tags: |
@@ -143,7 +158,7 @@
type=semver,pattern={{major}},enable=${{ startsWith(github.ref, 'refs/tags/v') }}
- name: 🚀 Build and push Docker image
- uses: docker/build-push-action@v5
+ uses: docker/build-push-action@v6
with:
context: .
file: ./Dockerfile
diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
--- a/.github/workflows/release.yml
+++ b/.github/workflows/release.yml
@@ -20,18 +20,19 @@
steps:
- name: Checkout repository
- uses: actions/checkout@v4
+ uses: actions/checkout@v6
with:
fetch-depth: 0 # Fetch all history for tags
token: ${{ secrets.RELEASE_TOKEN }}
- - name: Setup Bun
- uses: oven-sh/setup-bun@v2
+ - name: Setup Node.js
+ uses: actions/setup-node@v6
with:
- bun-version: latest
+ node-version: '24'
+ cache: npm
- name: Install dependencies
- run: bun install --frozen-lockfile
+ run: npm ci
- name: Configure Git
run: |
@@ -42,4 +43,4 @@
env:
GITHUB_TOKEN: ${{ secrets.RELEASE_TOKEN }}
GITHUB_REPOSITORY: ${{ github.repository }}
- run: bun scripts/release.ts ${{ inputs.semverType }}
+ run: node --import ./server/register-hooks.mjs ./scripts/release.ts ${{ inputs.semverType }}
diff --git a/.gitignore b/.gitignore
--- a/.gitignore
+++ b/.gitignore
@@ -1,7 +1,6 @@
# dependencies
/node_modules
-# we use bun
-package-lock.json
+# npm lockfile is committed
# dotenv environment variable files
.env
diff --git a/.oxlintrc.json b/.oxlintrc.json
new file mode 100644
--- /dev/null
+++ b/.oxlintrc.json
@@ -1,0 +1,7 @@
+{
+ "extends": ["./node_modules/@epic-web/config/oxlint-config.json"],
+ "rules": {
+ "import/consistent-type-specifier-style": "off",
+ "typescript/consistent-type-imports": "off"
+ }
+}
diff --git a/AGENTS.md b/AGENTS.md
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -1,18 +1,18 @@
-Default to using Bun instead of Node.js.
+Default to using Node.js and npm.
## Linting
-Always run `bun run lint` before you're done working to fix any lint issues.
+Always run `npm run lint` before you're done working to fix any lint issues.
## Formatting
-Always run `bun run format` before you're done working to fix any formatting issues.
+Always run `npm run format` before you're done working to fix any formatting issues.
## Commit Gate
Always run the full gate before committing:
-`bun run validate`
+`npm run validate`
Do not commit if any part of the gate fails.
@@ -331,36 +331,30 @@
If navigation regressions appear, debug the app code first (Link handling,
route registration, and state updates) before assuming a framework bug.
-## Bun
+## Node.js
-- Use `bun <file>` instead of `node <file>` or `ts-node <file>`
-- Use `bun test` instead of `jest` or `vitest`
-- Use `bun build <file.html|file.ts|file.css>` instead of `webpack` or `esbuild`
-- Use `bun install` instead of `npm install` or `yarn install` or `pnpm install`
-- Use `bun run <script>`
-- Bun automatically loads .env, so don't use dotenv.
-- Use `Bun.env` instead of `process.env` to access environment variables. Runtime changes to `process.env` may not be reflected in `Bun.env`, so always use `Bun.env` for consistency.
+- Use `node` for runtime execution and `npm` for package management.
+- Use `npm install` to add dependencies and update `package-lock.json`.
+- Use `npm run <script>` for project scripts.
+- Use `process.env` for environment variables.
## Build
-There is no build step. This is shipped as-is. Instead, we use Bun's built-in runtime typescript support and we do a runtime bundling of the client-side code in `server/bundling.ts`.
+There is no build step. This is shipped as-is. We use modern Node.js runtime TypeScript support for `.ts` files, a small Node hook for `.tsx` loading, and runtime bundling of the client-side code in `server/bundling.ts`.
## APIs
-- `Bun.serve()` supports WebSockets, HTTPS, and routes. Don't use `express`.
-- `bun:sqlite` for SQLite. Don't use `better-sqlite3`.
-- `Bun.redis` for Redis. Don't use `ioredis`.
-- `Bun.sql` for Postgres. Don't use `pg` or `postgres.js`.
+- Use the Node HTTP stack plus `remix/node-fetch-server` for the server entrypoint.
+- Use `node:sqlite` for SQLite.
- `WebSocket` is built-in. Don't use `ws`.
-- Prefer `Bun.file` over `node:fs`'s readFile/writeFile
-- Bun.$`ls` instead of execa.
+- Prefer Node's built-in `fs`, `fs/promises`, and web `File`/`Blob` APIs for file access.
## Testing
-Use `bun test` to run tests.
+Use `npm test` to run tests.
```ts#index.test.ts
-import { test, expect } from "bun:test";
+import { test, expect } from "vitest";
test("hello world", () => {
expect(1).toBe(1);
@@ -372,7 +366,7 @@
To test the application with sample media files, use the `local-test` directory:
```bash
-bun run dev:test
+npm run dev:testThis command: diff --git a/Dockerfile b/Dockerfile syntax=docker/dockerfile:1-FROM oven/bun:1 AS base Install FFmpeg for metadata editing@@ -8,8 +8,8 @@ Install dependenciesFROM base AS install Final imageFROM base AS release Run the application-CMD ["bun", "run", "start"] diff --git a/README.md b/README.md +## Development Tooling ScreenshotsDashboarddiff --git a/app/cache/cache.ts b/app/cache/cache.ts function ensureDirectoryExists(filePath: string): void {
function getGetStatement() { function getSetStatement() {
function getDeleteStatement() {
// Schema for validating cache entry metadata
diff --git a/app/client/admin/create-feed.tsx b/app/client/admin/create-feed.tsx
diff --git a/app/client/admin/edit-route-paths.test.ts b/app/client/admin/edit-route-paths.test.ts diff --git a/app/client/admin/feed-detail.tsx b/app/client/admin/feed-detail.tsx diff --git a/app/client/admin/feed-list.tsx b/app/client/admin/feed-list.tsx diff --git a/app/client/admin/media-detail.tsx b/app/client/admin/media-detail.tsx diff --git a/app/client/admin/media-list.tsx b/app/client/admin/media-list.tsx diff --git a/app/client/admin/version.tsx b/app/client/admin/version.tsx diff --git a/app/components/modal.tsx b/app/components/modal.tsx
diff --git a/app/config/env.ts b/app/config/env.ts
diff --git a/app/db/bun-sqlite-adapter-compat.ts b/app/db/bun-sqlite-adapter-compat.ts
type BunStatementResult = { diff --git a/app/db/feed-analytics-events.test.ts b/app/db/feed-analytics-events.test.ts diff --git a/app/db/feed-analytics-events.ts b/app/db/feed-analytics-events.ts export type CreateFeedAnalyticsEventData = { diff --git a/app/db/feeds.test.ts b/app/db/feeds.test.ts /**
diff --git a/app/db/index.ts b/app/db/index.ts function getDatabasePath(): string { diff --git a/app/db/migrations.analytics.test.ts b/app/db/migrations.analytics.test.ts diff --git a/app/db/migrations.ts b/app/db/migrations.ts type Migration = { diff --git a/app/db/sqlite.ts b/app/db/sqlite.ts
+type DatabaseParameters = Array | Record<string, unknown>
+function normalizeRunResult(result: StatementResultingChanges) {
+class PreparedStatement<TRow = Record<string, unknown>> {
+export class Database {
+export type StatementResult = ReturnType<PreparedStatement['run']> diff --git a/app/db/test-database.ts b/app/db/test-database.ts export function createMigratedTestDatabase(prefix: string): { diff --git a/app/helpers/analytics-request.test.ts b/app/helpers/analytics-request.test.ts diff --git a/app/helpers/analytics-window.test.ts b/app/helpers/analytics-window.test.ts function createRequest(search = ''): Request { diff --git a/app/helpers/bundle-version.ts b/app/helpers/bundle-version.ts
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Dummy request prevents range/conditional handling for artwork
- Updated feed artwork resolution to accept and pass through the real request so file responses can honor range/conditional headers.
2 participants
Test Plan
/admin/healthresponds.Checklist
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor