Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded stricter host/origin validation and null-byte path protection, refactored auto-import runner resolution with diagnostic-rich timeouts and process termination, bumped toktrack to 2.5.0, updated CI/dependency pins, expanded localized auto-import messages, and added extensive integration, unit, e2e and frontend tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Resolver as RunnerResolver
participant Runner as ToktrackRunner
participant Registry as BackgroundRegistry
Client->>Server: POST /api/auto-import (start)
Server->>Server: validateRequestHost(req)
alt host invalid
Server-->>Client: 403 Untrusted host header
else host valid
Server->>Resolver: resolveToktrackRunnerWithDiagnostics()
Resolver->>Runner: probe (local toktrack --version)
alt local ok
Resolver-->>Server: { runner: local }
else local mismatch/failure
Resolver->>Runner: probe package runners (bunx/npx)
Runner-->>Resolver: ok / timedOut / failed (messages)
Resolver-->>Server: diagnostics (localVersionMismatch/localFailure/runnerFailures)
end
Server->>Runner: run toktrack daily --json (with timeouts)
Runner-->>Server: stream events / JSON or error
Server->>Registry: store background instance (id, port, apiPrefix)
Server-->>Client: SSE/events or mapped error (messageKey/messageVars)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 docstrings
🧪 Generate unit tests (beta)
Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/http-utils.js (1)
181-190:⚠️ Potential issue | 🟡 MinorNormalize
Hostbefore comparing it toOrigin.
validateRequestHost()lowercases/parses the host name, buthasTrustedOrigin()still comparesorigin.hostagainst the rawHostheader. A request likeHost: LOCALHOST:3000withOrigin: http://localhost:3000will pass the host gate and still get a 403 here, even though host names are case-insensitive.Suggested fix
function hasTrustedOrigin(req) { const originHeader = getHeaderValue(req, 'origin').trim(); - const hostHeader = getHeaderValue(req, 'host').trim(); + const hostHeader = getHeaderValue(req, 'host').trim().toLowerCase(); if (!originHeader || !hostHeader || originHeader === 'null') { return false; } try { const origin = new URL(originHeader); - return origin.host === hostHeader; + return origin.host.toLowerCase() === hostHeader; } catch { return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/http-utils.js` around lines 181 - 190, hasTrustedOrigin() compares origin.host to the raw Host header which may differ in case; normalize the Host header the same way validateRequestHost() does (lowercase and parse/normalize the hostname:port) before comparing to new URL(originHeader).host (or call validateRequestHost(req) to get the normalized host) so case-insensitive matches like "LOCALHOST:3000" vs "localhost:3000" succeed; ensure you still trim and handle missing/null headers and keep the existing try/catch behavior in hasTrustedOrigin().
🧹 Nitpick comments (1)
tests/integration/server.test.ts (1)
733-745: Avoid hardcoding the toktrack version in these fake runner scripts.These fixtures will break on the next version bump even if the behavior stays correct. Reusing
TOKTRACK_VERSIONkeeps the tests aligned with the pinned package version automatically.Suggested refactor
import sampleUsage from '../../examples/sample-usage.json' +import { TOKTRACK_VERSION } from '../../shared/toktrack-version.js' import { DEFAULT_DASHBOARD_FILTERS, getDefaultDashboardSectionOrder, } from '@/lib/dashboard-preferences'- ' echo "toktrack 2.5.0"', + ` echo "toktrack ${TOKTRACK_VERSION}"`,- ' console.log("toktrack 2.5.0")', + ` console.log("toktrack ${TOKTRACK_VERSION}")`,Also applies to: 785-806
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/server.test.ts` around lines 733 - 745, The test fixture hardcodes the toktrack version string ("toktrack 2.5.0") in the fake runner created via writeFileSync at fakeToktrackPath; update the script content to use the shared TOKTRACK_VERSION constant instead (e.g., construct the "--version" echo as `echo "toktrack ${TOKTRACK_VERSION}"`) and apply the same replacement to the other fake runner usage noted (the block around lines 785-806) so the tests follow the pinned TOKTRACK_VERSION rather than a hardcoded literal; ensure chmodSync stays the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/dependabot.yml:
- Around line 6-9: Update the Dependabot configuration to explicitly ignore
major version updates for eslint and `@eslint/js` by adding an "ignore" entry
under the npm package-ecosystem section that lists dependency-name: "eslint" and
dependency-name: "@eslint/js" with versions set to ">=10" (or a pattern that
blocks v10+) and update-types including "version-update:semver-major"; this
keeps your documented policy ("keep eslint/@eslint/js on 9.x") enforced while
leaving existing minor/patch update rules in place.
In `@server.js`:
- Around line 1908-1920: The timeout branch currently calls
terminateChildProcess(child) and immediately rejects, which allows
performAutoImport() to clear autoImportRunning before the timed-out child
actually exits; change the timeout handler so it waits for the child process to
exit before calling settle(reject,...). Specifically, in the timeout callback
(where terminateChildProcess(child) is invoked and settle is called with
createCommandError), replace the immediate reject with logic that (a) call
terminateChildProcess(child) and then wait for the child's 'exit' event (or
await a promise returned by terminateChildProcess if it provides one), (b) once
the child has exited, call settle(reject, createCommandError(...)) with the same
payload, and (c) ensure any temporary listeners are cleaned up; this ensures
autoImportRunning isn't cleared until the timed-out child has actually
terminated.
---
Outside diff comments:
In `@server/http-utils.js`:
- Around line 181-190: hasTrustedOrigin() compares origin.host to the raw Host
header which may differ in case; normalize the Host header the same way
validateRequestHost() does (lowercase and parse/normalize the hostname:port)
before comparing to new URL(originHeader).host (or call validateRequestHost(req)
to get the normalized host) so case-insensitive matches like "LOCALHOST:3000" vs
"localhost:3000" succeed; ensure you still trim and handle missing/null headers
and keep the existing try/catch behavior in hasTrustedOrigin().
---
Nitpick comments:
In `@tests/integration/server.test.ts`:
- Around line 733-745: The test fixture hardcodes the toktrack version string
("toktrack 2.5.0") in the fake runner created via writeFileSync at
fakeToktrackPath; update the script content to use the shared TOKTRACK_VERSION
constant instead (e.g., construct the "--version" echo as `echo "toktrack
${TOKTRACK_VERSION}"`) and apply the same replacement to the other fake runner
usage noted (the block around lines 785-806) so the tests follow the pinned
TOKTRACK_VERSION rather than a hardcoded literal; ensure chmodSync stays the
same.
🪄 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: d165959b-465c-49f5-a93a-3d3a9f81ef09
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
.github/dependabot.yml.github/workflows/ci.yml.github/workflows/release.yml.gitignoreCHANGELOG.mdREADME.mdpackage.jsonserver.jsserver/http-utils.jsshared/toktrack-version.d.tsshared/toktrack-version.jssrc/components/features/auto-import/AutoImportModal.tsxsrc/lib/auto-import.tssrc/locales/de/common.jsonsrc/locales/en/common.jsontests/e2e/dashboard.spec.tstests/frontend/auto-import-modal.test.tsxtests/frontend/settings-modal.test.tsxtests/integration/server.test.tstests/unit/api.test.tstests/unit/auto-import.test.tstests/unit/http-utils.test.tstests/unit/server-helpers.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/auto-import.test.ts (1)
27-28: Add a focused assertion fortoktrackInvalidDatatranslation.Line 27 defines the
toktrackInvalidDatamessage, but there’s no directtranslateAutoImportEvent({ key: 'toktrackInvalidData' ... })assertion alongside the new error-event checks. Adding oneexpecthere would close that regression gap.As per coding guidelines: "Prefer focused
*.test.tsor*.test.tsxcoverage for data transforms, hooks, or complex UI behavior".Also applies to: 104-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/auto-import.test.ts` around lines 27 - 28, Add a focused assertion that directly tests the toktrackInvalidData translation by calling translateAutoImportEvent with key 'toktrackInvalidData' and a sample params.message, then expect the returned string to equal the German message defined under 'autoImportModal.toktrackInvalidData'; place this expect near the other new error-event checks in tests/unit/auto-import.test.ts (and add the same focused assertion in the other block mentioned around the later error-event checks) so the test covers translateAutoImportEvent({ key: 'toktrackInvalidData', params: { message: '...' } }) against the 'autoImportModal.toktrackInvalidData' message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/auto-import.test.ts`:
- Around line 27-28: Add a focused assertion that directly tests the
toktrackInvalidData translation by calling translateAutoImportEvent with key
'toktrackInvalidData' and a sample params.message, then expect the returned
string to equal the German message defined under
'autoImportModal.toktrackInvalidData'; place this expect near the other new
error-event checks in tests/unit/auto-import.test.ts (and add the same focused
assertion in the other block mentioned around the later error-event checks) so
the test covers translateAutoImportEvent({ key: 'toktrackInvalidData', params: {
message: '...' } }) against the 'autoImportModal.toktrackInvalidData' message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03846534-5088-4432-b698-cbaf9aabde2f
📒 Files selected for processing (11)
.github/dependabot.ymlCHANGELOG.mdserver.jsserver/http-utils.jssrc/lib/auto-import.tssrc/locales/de/common.jsonsrc/locales/en/common.jsontests/integration/server.test.tstests/unit/auto-import.test.tstests/unit/http-utils.test.tstests/unit/server-helpers.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/dependabot.yml
- src/lib/auto-import.ts
- src/locales/de/common.json
- CHANGELOG.md
- tests/unit/http-utils.test.ts
- src/locales/en/common.json
- server.js
- tests/unit/server-helpers.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Documentation