chore(runtime-core): Add deepnote runtime-core dependency, and use it instead of existing implementation#355
Open
chore(runtime-core): Add deepnote runtime-core dependency, and use it instead of existing implementation#355
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMigrates Deepnote server lifecycle to Sequence Diagram(s)sequenceDiagram
participant Extension as "VS Code Extension\n(DeepnoteServerStarter)"
participant Runtime as "runtime-core\n(startServer / stopServer)"
participant SQL as "SQL Env Provider\n(optional)"
participant Output as "VS Code OutputChannel"
participant Network as "Server URL / Health"
Extension->>SQL: request SQL env vars (optional)
SQL-->>Extension: sqlEnvVars / cancelled
Extension->>Runtime: startServer({interpreter, venvPath, additionalPackages, env: sqlEnvVars})
Runtime-->>Extension: ServerInfo (url, ports, id, processRef)
Extension->>Output: monitorServerOutput(processRef)
Extension->>Network: probe ServerInfo.url for health
Network-->>Extension: healthy/unhealthy
Extension->>Runtime: stopServer(ServerInfo.id)
Runtime-->>Extension: stopped
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/kernels/deepnote/deepnoteServerStarter.node.ts`:
- Around line 258-259: reserveStartPort(serverKey) currently only serializes
maxPort computation and releases the lock before server startup, allowing
duplicate base-port assignments; modify the flow so the reservation is recorded
and held until startup completes (or explicitly tracked and released) — e.g.,
have reserveStartPort return and persist a concrete reservation object/port for
serverKey and only release or remove that reservation after
startServer/launchServer (or the function that calls reserveStartPort) finishes
or fails, and ensure the locking in the maxPort calculation (the block around
calculateMaxPort/getMaxPort) still wraps reservation creation to prevent races;
update related callers (reserveStartPort, startServer/launchServer, and any
unlock/release logic around lines referenced) to use the reservation lifecycle
instead of releasing the lock immediately.
- Around line 298-299: The output listeners are being stored under serverKey
while stopServerForEnvironment looks them up by fileKey, causing leaked
listeners; update the monitorServerOutput path (where disposables are set at
lines 442-445) to store the disposables using the same key
stopServerForEnvironment expects (fileKey) or alternatively change
stopServerForEnvironment to use serverKey—make the key consistent between
monitorServerOutput (and where disposables are assigned) and
stopServerForEnvironment so listeners are removed correctly; reference
monitorServerOutput, stopServerForEnvironment, serverKey, fileKey and the
disposables map when making the change.
- Around line 266-277: The cancellation path is lost between
gatherSqlIntegrationEnvVars(...) and startServer(...): update
gatherSqlIntegrationEnvVars to propagate/throw an OperationCanceled (or the same
cancellation) instead of swallowing it, then after startServer(...) check for
cancellation (e.g. via the same token or a isCancelled check) and if canceled,
call the runtime shutdown/teardown routine on the returned RuntimeCoreServerInfo
(or call the server stop/close function provided by startServer) before
re-throwing the cancellation; also ensure the earlier provider-failure fallback
that returns {} (from the SQL provider logic) does not hide cancellations by
converting cancellation errors into non-cancel errors so that cancellation
bubbles up consistently.
- Around line 481-485: The code is passing wrapper objects from
pendingOperations to Promise.race instead of their promises, so change the await
to use each wrapper's promise: map pendingOps to op.promise (or a safe fallback
if missing) when creating the Promise.race with
sleep(GRACEFUL_SHUTDOWN_TIMEOUT_MS) so the actual async operations tracked in
pendingOperations are awaited before dispose clears state; locate
pendingOperations/pendingOps and the Promise.race call to update it.
In `@src/kernels/deepnote/deepnoteServerStarter.unit.test.ts`:
- Around line 85-99: The test "should serialize concurrent calls" currently
allows duplicate ports and won't catch races in reserveStartPort; update the
test to assert that concurrent calls to
reserveStartPort('key-1'/'key-2'/'key-3') return distinct/non-overlapping port
numbers (or replace the test to call the public startup flow that uses
reserveStartPort) so duplicate reservations fail the test; locate
reserveStartPort in deepnoteServerStarter and change assertions in the unit test
to enforce uniqueness of port1, port2, port3 (or exercise the public start
method) to surface concurrency issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b086b66-7c45-4d48-87d4-78e3a43b7520
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.jsonsrc/kernels/deepnote/deepnoteServerStarter.node.tssrc/kernels/deepnote/deepnoteServerStarter.unit.test.tssrc/kernels/deepnote/deepnoteToolkitInstaller.node.tssrc/kernels/deepnote/types.ts
Base automatically changed from
tk/update-deepnote-database-integrations
to
main
March 16, 2026 13:59
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #355 +/- ##
===========================
===========================
🚀 New features to boost your workflow:
|
…ss helper - Introduced `createMockChildProcess` in `deepnoteTestHelpers.ts` for consistent mock process creation in tests. - Updated `DeepnoteLspClientManager` and `DeepnoteServerStarter` to include the mock process in server info. - Removed unnecessary `runtimeCoreServerInfo` from `ProjectContext` and adjusted related logic to use the new `serverInfo` structure. - Ensured all relevant tests are updated to reflect these changes, improving test reliability and maintainability.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/kernels/deepnote/deepnoteServerStarter.node.ts (2)
474-474:⚠️ Potential issue | 🔴 Critical
Promise.racereceives wrapper objects, not promises.
pendingOpscontains{ type, promise }objects. This immediately resolves with the wrapper. Useop.promise.-await Promise.allSettled(pendingOps.map((op) => Promise.race([op, sleep(GRACEFUL_SHUTDOWN_TIMEOUT_MS)]))); +await Promise.allSettled(pendingOps.map((op) => Promise.race([op.promise, sleep(GRACEFUL_SHUTDOWN_TIMEOUT_MS)])));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteServerStarter.node.ts` at line 474, The current shutdown waiter passes the wrapper objects from pendingOps into Promise.race, causing immediate resolution; update the Promise.race call inside the Promise.allSettled to use each wrapper's promise property (i.e., change pendingOps.map((op) => Promise.race([op, sleep(GRACEFUL_SHUTDOWN_TIMEOUT_MS)])) to use op.promise) so that Promise.race races the actual operation promise against sleep(GRACEFUL_SHUTDOWN_TIMEOUT_MS).
432-435:⚠️ Potential issue | 🟠 MajorDisposables keyed by
serverKeybut cleanup usesfileKey.Line 342 looks up disposables by
fileKey, but line 435 stores them underserverKey. Listeners will leak on notebook close.- private monitorServerOutput(serverKey: string, serverInfo: DeepnoteServerInfo): void { + private monitorServerOutput(fileKey: string, serverInfo: DeepnoteServerInfo): void { const proc = serverInfo.process; const disposables: IDisposable[] = []; - this.disposablesByFile.set(serverKey, disposables); + this.disposablesByFile.set(fileKey, disposables);Update the call site at line 290 to pass
fileKeyinstead ofserverKey.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteServerStarter.node.ts` around lines 432 - 435, The disposables are stored in disposablesByFile using serverKey in monitorServerOutput (method monitorServerOutput and property disposablesByFile) but cleanup looks them up by fileKey, causing listener leaks; update the caller that invokes monitorServerOutput (the call passing serverKey) to pass the notebook's fileKey instead of serverKey so disposables are indexed by the same fileKey used for cleanup, and verify references to serverKey/fileKey across monitorServerOutput, disposablesByFile, and the cleanup lookup are consistent.src/kernels/deepnote/deepnoteServerStarter.unit.test.ts (1)
90-104: 🧹 Nitpick | 🔵 TrivialTest doesn't verify distinct port reservations.
All three calls return the same port since no server info is stored between reservations. Consider asserting uniqueness or testing the full startup flow to catch races.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteServerStarter.unit.test.ts` around lines 90 - 104, The test currently calls the private method reserveStartPort concurrently and only checks that returned values are numbers, but it doesn't catch the race that returns the same port; update the test to verify uniqueness (e.g., collect [port1, port2, port3] into a Set and assert its size is 3) or, better, exercise the full startup flow that records server info (use serverStarter.startServer or the public method that persists server metadata) so concurrent reservations produce distinct ports; locate the private helper reserveStartPort via getPrivateMethod(serverStarter, 'reserveStartPort') and replace the final assertions with a uniqueness assertion or a full-start integration assertion to ensure distinct port allocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/kernels/deepnote/deepnoteTestHelpers.ts`:
- Line 1: Replace the Node-prefixed import for the ChildProcess type with the
standard module path: change the import statement that currently references
'node:child_process' and the type symbol ChildProcess to import from
'child_process' instead, ensuring all usages of the ChildProcess type in this
file (e.g., in deepnoteTestHelpers.ts) remain the same.
---
Duplicate comments:
In `@src/kernels/deepnote/deepnoteServerStarter.node.ts`:
- Line 474: The current shutdown waiter passes the wrapper objects from
pendingOps into Promise.race, causing immediate resolution; update the
Promise.race call inside the Promise.allSettled to use each wrapper's promise
property (i.e., change pendingOps.map((op) => Promise.race([op,
sleep(GRACEFUL_SHUTDOWN_TIMEOUT_MS)])) to use op.promise) so that Promise.race
races the actual operation promise against sleep(GRACEFUL_SHUTDOWN_TIMEOUT_MS).
- Around line 432-435: The disposables are stored in disposablesByFile using
serverKey in monitorServerOutput (method monitorServerOutput and property
disposablesByFile) but cleanup looks them up by fileKey, causing listener leaks;
update the caller that invokes monitorServerOutput (the call passing serverKey)
to pass the notebook's fileKey instead of serverKey so disposables are indexed
by the same fileKey used for cleanup, and verify references to serverKey/fileKey
across monitorServerOutput, disposablesByFile, and the cleanup lookup are
consistent.
In `@src/kernels/deepnote/deepnoteServerStarter.unit.test.ts`:
- Around line 90-104: The test currently calls the private method
reserveStartPort concurrently and only checks that returned values are numbers,
but it doesn't catch the race that returns the same port; update the test to
verify uniqueness (e.g., collect [port1, port2, port3] into a Set and assert its
size is 3) or, better, exercise the full startup flow that records server info
(use serverStarter.startServer or the public method that persists server
metadata) so concurrent reservations produce distinct ports; locate the private
helper reserveStartPort via getPrivateMethod(serverStarter, 'reserveStartPort')
and replace the final assertions with a uniqueness assertion or a full-start
integration assertion to ensure distinct port allocation.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1bd083f2-2827-4d07-bfdf-f36afa80cc25
📒 Files selected for processing (7)
src/kernels/deepnote/deepnoteLspClientManager.node.vscode.test.tssrc/kernels/deepnote/deepnoteServerStarter.node.tssrc/kernels/deepnote/deepnoteServerStarter.unit.test.tssrc/kernels/deepnote/deepnoteTestHelpers.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.tssrc/kernels/deepnote/types.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/kernels/deepnote/deepnoteServerStarter.unit.test.ts (1)
90-104: 🧹 Nitpick | 🔵 TrivialConcurrency test verifies serialization but not uniqueness.
Test confirms no crash on concurrent calls. Since
reserveStartPortonly reads existingprojectContexts(none here), all return 8888. To truly test uniqueness, inject mock contexts between calls or test the full startup path.Current behavior is correct per implementation—just noting for coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteServerStarter.unit.test.ts` around lines 90 - 104, The test for reserveStartPort only ensures concurrent calls don't crash but doesn't assert uniqueness; update the test to simulate occupied ports (e.g., inject/mock serverStarter.projectContexts or the method that reads projectContexts) or call the full startup path that registers servers so concurrent reservations must pick unique ports; specifically modify the unit test around reserveStartPort to pre-populate/mock projectContexts with existing ports (or spy on startNotebookServer/startServer registration) and then assert the returned ports are distinct instead of merely numbers.src/kernels/deepnote/deepnoteTestHelpers.node.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorUse
child_processwithout thenode:prefix.-import type { ChildProcess } from 'node:child_process'; +import type { ChildProcess } from 'child_process';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteTestHelpers.node.ts` at line 1, Replace the node-prefixed builtin import with the plain module name: change the import of the type ChildProcess from 'node:child_process' to 'child_process' (i.e., update the import statement that references ChildProcess in deepnoteTestHelpers.node.ts and any other occurrences to use 'child_process' instead of 'node:child_process').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/kernels/deepnote/deepnoteServerStarter.unit.test.ts`:
- Around line 90-104: The test for reserveStartPort only ensures concurrent
calls don't crash but doesn't assert uniqueness; update the test to simulate
occupied ports (e.g., inject/mock serverStarter.projectContexts or the method
that reads projectContexts) or call the full startup path that registers servers
so concurrent reservations must pick unique ports; specifically modify the unit
test around reserveStartPort to pre-populate/mock projectContexts with existing
ports (or spy on startNotebookServer/startServer registration) and then assert
the returned ports are distinct instead of merely numbers.
In `@src/kernels/deepnote/deepnoteTestHelpers.node.ts`:
- Line 1: Replace the node-prefixed builtin import with the plain module name:
change the import of the type ChildProcess from 'node:child_process' to
'child_process' (i.e., update the import statement that references ChildProcess
in deepnoteTestHelpers.node.ts and any other occurrences to use 'child_process'
instead of 'node:child_process').
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0cebc08-6b64-49c2-8cb5-b661870a8068
📒 Files selected for processing (5)
src/kernels/deepnote/deepnoteLspClientManager.node.vscode.test.tssrc/kernels/deepnote/deepnoteServerStarter.unit.test.tssrc/kernels/deepnote/deepnoteTestHelpers.node.tssrc/kernels/deepnote/environments/deepnoteEnvironmentTreeDataProvider.unit.test.tssrc/notebooks/deepnote/deepnoteKernelAutoSelector.node.unit.test.ts
- Added a warning log when no project context is found, preventing server stop attempts. - Updated the `stopServerForEnvironment` method to require a non-null project context, ensuring safer operation handling.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/kernels/deepnote/deepnoteServerStarter.node.ts (2)
85-177: 🧹 Nitpick | 🔵 TrivialMethod ordering doesn't follow guideline.
Per coding guidelines, methods should be ordered by accessibility then alphabetically.
dispose(line 473) should precedestartServer(line 99).As per coding guidelines: "Order method, fields and properties, first by accessibility and then by alphabetical order"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteServerStarter.node.ts` around lines 85 - 177, The class method ordering violates the project's "accessibility then alphabetical" rule: move the public dispose method so it appears before startServer; locate the dispose method (named dispose) and the startServer method (named startServer) and cut/paste dispose to precede startServer in the class body, keeping its implementation and any related imports/fields intact and ensuring no references or pendingOperations logic are changed.
182-218:⚠️ Potential issue | 🔴 CriticalKey mismatch between
startServerandstopServer.
startServerusesserverKey(${fileKey}-${environmentId}) forprojectContextsandpendingOperations, butstopServeruses barefileKey. This means:
- Line 186 won't find contexts created by
startServer- Line 193 won't detect pending start operations
- Stop operations could incorrectly conflict
Proposed fix
Either pass
environmentIdtostopServerand useserverKey, or document thatstopServerstops all servers for a file (and iterate over matching keys).- public async stopServer(deepnoteFileUri: Uri, token?: CancellationToken): Promise<void> { + public async stopServer(deepnoteFileUri: Uri, environmentId: string, token?: CancellationToken): Promise<void> { Cancellation.throwIfCanceled(token); const fileKey = deepnoteFileUri.fsPath; - const projectContext = this.projectContexts.get(fileKey) ?? null; + const serverKey = `${fileKey}-${environmentId}`; + const projectContext = this.projectContexts.get(serverKey) ?? null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteServerStarter.node.ts` around lines 182 - 218, stopServer currently looks up projectContexts and pendingOperations by fileKey but startServer stores them under serverKey (`${fileKey}-${environmentId}`), so stopServer won't find matching contexts or pending ops and can mis-handle concurrency; fix by either (A) adding an environmentId parameter to stopServer and change lookups/sets to use serverKey (mirror how startServer creates entries in projectContexts and pendingOperations), ensuring you pass serverKey into stopServerForEnvironment, or (B) if the intent is to stop all environments for a file, iterate over projectContexts.keys() and pendingOperations.keys(), select keys that startWith(fileKey) and perform the same wait/stop logic for each matching serverKey (using the existing stopServerForEnvironment and clearing pendingOperations/projectContexts for those serverKeys).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/kernels/deepnote/deepnoteServerStarter.node.ts`:
- Around line 281-290: The catch block currently discards any captured process
output and only passes error.message to DeepnoteServerStartupError; instead,
inspect the thrown error for captured output fields (e.g., error.stdout,
error.stderr, error.output, or nested properties), extract relevant
stdout/stderr (falling back to error.message or String(error)), and pass those
values into DeepnoteServerStartupError so the constructor receives the real
stdout and stderr content (referencing DeepnoteServerStartupError,
interpreter.uri.fsPath, and port) and include the original Error object as
before.
- Around line 357-364: The isServerRunning method currently calls
fetch(`${serverInfo.url}/api`) with no timeout, which can hang; update
isServerRunning to use an AbortSignal-based timeout (e.g., AbortSignal.timeout
or an AbortController with setTimeout) and pass the signal to fetch via the
options object so the request is aborted after a reasonable timeout (e.g.,
3–5s); ensure you clean up any controller timers if using AbortController and
handle the aborted error path to return false from isServerRunning.
- Around line 335-337: The conditional check around resetting
projectContext.serverInfo is redundant because projectContext is a required
parameter and cannot be null; in the function/method where the block appears
(look for the code that sets projectContext.serverInfo to null in
deepnoteServerStarter.node.ts), remove the surrounding "if (projectContext) {
... }" and directly assign projectContext.serverInfo = null so the intent
remains but without the unnecessary null-check.
---
Outside diff comments:
In `@src/kernels/deepnote/deepnoteServerStarter.node.ts`:
- Around line 85-177: The class method ordering violates the project's
"accessibility then alphabetical" rule: move the public dispose method so it
appears before startServer; locate the dispose method (named dispose) and the
startServer method (named startServer) and cut/paste dispose to precede
startServer in the class body, keeping its implementation and any related
imports/fields intact and ensuring no references or pendingOperations logic are
changed.
- Around line 182-218: stopServer currently looks up projectContexts and
pendingOperations by fileKey but startServer stores them under serverKey
(`${fileKey}-${environmentId}`), so stopServer won't find matching contexts or
pending ops and can mis-handle concurrency; fix by either (A) adding an
environmentId parameter to stopServer and change lookups/sets to use serverKey
(mirror how startServer creates entries in projectContexts and
pendingOperations), ensuring you pass serverKey into stopServerForEnvironment,
or (B) if the intent is to stop all environments for a file, iterate over
projectContexts.keys() and pendingOperations.keys(), select keys that
startWith(fileKey) and perform the same wait/stop logic for each matching
serverKey (using the existing stopServerForEnvironment and clearing
pendingOperations/projectContexts for those serverKeys).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4a76f561-b8cb-437c-82d4-06797d18993c
📒 Files selected for processing (1)
src/kernels/deepnote/deepnoteServerStarter.node.ts
- Updated the DeepnoteServerStarter class to consistently use fileKey for managing pending operations and project contexts, improving clarity and reducing potential errors. - Adjusted logging messages to reflect the change, ensuring accurate information is logged during server operations.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/kernels/deepnote/deepnoteServerStarter.node.ts (6)
424-426:⚠️ Potential issue | 🟠 MajorCancellation swallowed in catch block.
If
getEnvironmentVariablesthrows due to cancellation, it's logged and ignored. Check for cancellation errors and re-throw.Proposed fix
} catch (error) { + if (Cancellation.isCanceled(error)) { + throw error; + } logger.error('DeepnoteServerStarter: Failed to get SQL integration env vars', error); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteServerStarter.node.ts` around lines 424 - 426, The catch in DeepnoteServerStarter around getEnvironmentVariables swallows cancellation errors; update the catch to detect cancellation/abort errors (e.g., inspect error.name === 'AbortError' or error.code === 'ERR_ABORTED' or other library-specific cancellation marker) and re-throw them instead of continuing, while still logging non-cancellation errors via logger.error; ensure the logic is applied to the try/catch that calls getEnvironmentVariables so cancellation propagates properly.
280-288: 🧹 Nitpick | 🔵 TrivialConsider extracting captured output from error.
If runtime-core's error includes
stdout/stderrproperties, they're discarded. Could improve diagnostics.Proposed enhancement
} catch (error) { + const stdout = error instanceof Error && 'stdout' in error ? String(error.stdout) : ''; + const stderr = error instanceof Error && 'stderr' in error ? String(error.stderr) : (error instanceof Error ? error.message : String(error)); throw new DeepnoteServerStartupError( interpreter.uri.fsPath, port, 'unknown', - '', - error instanceof Error ? error.message : String(error), + stdout, + stderr, error instanceof Error ? error : undefined ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteServerStarter.node.ts` around lines 280 - 288, The catch block that throws DeepnoteServerStartupError currently only forwards message and error object but drops any runtime-core diagnostic output; update the catch in deepnoteServerStarter.node.ts to detect stdout/stderr (e.g. via const stdout = (error as any).stdout; const stderr = (error as any).stderr) and pass them into the DeepnoteServerStartupError (or append them to the error message) so interpreter.uri.fsPath, port and the original Error are preserved while including stdout/stderr for better diagnostics; modify the DeepnoteServerStartupError constructor call sites (or its signature if needed) to accept and propagate these stdout/stderr values.
473-477:⚠️ Potential issue | 🔴 Critical
Promise.race()receives wrapper objects, not promises.
pendingOpscontains{ type, promise }wrappers. PassingoptoPromise.race()resolves immediately with the wrapper. Useop.promise.Proposed fix
- await Promise.allSettled(pendingOps.map((op) => Promise.race([op, sleep(GRACEFUL_SHUTDOWN_TIMEOUT_MS)]))); + await Promise.allSettled(pendingOps.map((op) => Promise.race([op.promise, sleep(GRACEFUL_SHUTDOWN_TIMEOUT_MS)])));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteServerStarter.node.ts` around lines 473 - 477, The shutdown logic races each pending operation against the timeout but currently passes the wrapper objects from pendingOperations (pendingOps entries like { type, promise }) into Promise.race, causing immediate resolution; update the mapping to use the actual Promise property (use op.promise) so Promise.race([op.promise, sleep(GRACEFUL_SHUTDOWN_TIMEOUT_MS)]) is awaited for each entry, keeping the existing logger, sleep, and GRACEFUL_SHUTDOWN_TIMEOUT_MS symbols intact.
354-361:⚠️ Potential issue | 🟡 MinorAdd timeout to health check fetch.
fetchwithout timeout can hang indefinitely. UseAbortSignal.timeout().Proposed fix
private async isServerRunning(serverInfo: DeepnoteServerInfo): Promise<boolean> { try { - const response = await fetch(`${serverInfo.url}/api`); + const response = await fetch(`${serverInfo.url}/api`, { + signal: AbortSignal.timeout(5000) + }); return response.ok; } catch { return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteServerStarter.node.ts` around lines 354 - 361, The isServerRunning method calls fetch without a timeout; update isServerRunning(DeepnoteServerInfo) to create an AbortSignal via AbortSignal.timeout(<ms>) and pass that signal into fetch (fetch(`${serverInfo.url}/api`, { signal })), then catch aborted requests (and other errors) and return false; ensure the signal is properly used and cleaned up (no extra unhandled promise) so the health check always times out instead of hanging indefinitely.
370-394:⚠️ Potential issue | 🟠 MajorPort reservation race window remains.
Lock releases before
startServerbinds. Concurrent starts between lock release (line 392) and actual port binding can receive the same base port. Ifruntime-corehandles conflicts internally, this is mitigated; otherwise, consider holding the lock until startup completes or tracking reserved ports explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteServerStarter.node.ts` around lines 370 - 394, reserveStartPort currently releases portAllocationLock before the server actually binds, creating a race; fix by tracking reserved start ports explicitly instead of relying solely on the lock: add a module-level Set (e.g., reservedStartPorts) and in reserveStartPort (keep name) mark the chosen maxPort as reserved before releasing the lock, skipping any ports already present in reservedStartPorts or projectContexts' serverInfo; then update the server startup flow (startServer or whatever calls reserveStartPort) to remove the port from reservedStartPorts once binding succeeds or on failure so future callers can reuse it; ensure all references to portAllocationLock, projectContexts, and serverInfo continue to coordinate with the new reservedStartPorts tracking.
269-279:⚠️ Potential issue | 🟠 MajorMissing cancellation check before
startServer.After
gatherSqlIntegrationEnvVarsreturns, cancellation isn't checked before starting the server. If cancelled while gathering env vars, startup proceeds anyway.Proposed fix
const extraEnv = await this.gatherSqlIntegrationEnvVars(deepnoteFileUri, environmentId, token); + Cancellation.throwIfCanceled(token); + let serverInfo: DeepnoteServerInfo; try { serverInfo = await startServer({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteServerStarter.node.ts` around lines 269 - 279, After awaiting gatherSqlIntegrationEnvVars(deepnoteFileUri, environmentId, token) add an immediate check for cancellation using the provided token (e.g., if (token.isCancellationRequested) or the project's cancel-check helper) and bail out (throw or return) before calling startServer; this ensures startServer(...) is not invoked when the operation was cancelled. Update the method surrounding startServer and any serverInfo assignment accordingly (the symbols to edit are gatherSqlIntegrationEnvVars and startServer in the current method).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/kernels/deepnote/deepnoteServerStarter.node.ts`:
- Around line 65-70: Reorder the private readonly field declarations so they
follow alphabetical order within the same accessibility group; specifically, in
deepnoteServerStarter.node.ts reorder the fields disposablesByFile, lockFileDir,
pendingOperations, portAllocationLock, projectContexts, sessionId (all declared
as private readonly) into alphabetical order while preserving the accessibility
and readonly modifiers and keeping their initializers unchanged.
---
Duplicate comments:
In `@src/kernels/deepnote/deepnoteServerStarter.node.ts`:
- Around line 424-426: The catch in DeepnoteServerStarter around
getEnvironmentVariables swallows cancellation errors; update the catch to detect
cancellation/abort errors (e.g., inspect error.name === 'AbortError' or
error.code === 'ERR_ABORTED' or other library-specific cancellation marker) and
re-throw them instead of continuing, while still logging non-cancellation errors
via logger.error; ensure the logic is applied to the try/catch that calls
getEnvironmentVariables so cancellation propagates properly.
- Around line 280-288: The catch block that throws DeepnoteServerStartupError
currently only forwards message and error object but drops any runtime-core
diagnostic output; update the catch in deepnoteServerStarter.node.ts to detect
stdout/stderr (e.g. via const stdout = (error as any).stdout; const stderr =
(error as any).stderr) and pass them into the DeepnoteServerStartupError (or
append them to the error message) so interpreter.uri.fsPath, port and the
original Error are preserved while including stdout/stderr for better
diagnostics; modify the DeepnoteServerStartupError constructor call sites (or
its signature if needed) to accept and propagate these stdout/stderr values.
- Around line 473-477: The shutdown logic races each pending operation against
the timeout but currently passes the wrapper objects from pendingOperations
(pendingOps entries like { type, promise }) into Promise.race, causing immediate
resolution; update the mapping to use the actual Promise property (use
op.promise) so Promise.race([op.promise, sleep(GRACEFUL_SHUTDOWN_TIMEOUT_MS)])
is awaited for each entry, keeping the existing logger, sleep, and
GRACEFUL_SHUTDOWN_TIMEOUT_MS symbols intact.
- Around line 354-361: The isServerRunning method calls fetch without a timeout;
update isServerRunning(DeepnoteServerInfo) to create an AbortSignal via
AbortSignal.timeout(<ms>) and pass that signal into fetch
(fetch(`${serverInfo.url}/api`, { signal })), then catch aborted requests (and
other errors) and return false; ensure the signal is properly used and cleaned
up (no extra unhandled promise) so the health check always times out instead of
hanging indefinitely.
- Around line 370-394: reserveStartPort currently releases portAllocationLock
before the server actually binds, creating a race; fix by tracking reserved
start ports explicitly instead of relying solely on the lock: add a module-level
Set (e.g., reservedStartPorts) and in reserveStartPort (keep name) mark the
chosen maxPort as reserved before releasing the lock, skipping any ports already
present in reservedStartPorts or projectContexts' serverInfo; then update the
server startup flow (startServer or whatever calls reserveStartPort) to remove
the port from reservedStartPorts once binding succeeds or on failure so future
callers can reuse it; ensure all references to portAllocationLock,
projectContexts, and serverInfo continue to coordinate with the new
reservedStartPorts tracking.
- Around line 269-279: After awaiting
gatherSqlIntegrationEnvVars(deepnoteFileUri, environmentId, token) add an
immediate check for cancellation using the provided token (e.g., if
(token.isCancellationRequested) or the project's cancel-check helper) and bail
out (throw or return) before calling startServer; this ensures startServer(...)
is not invoked when the operation was cancelled. Update the method surrounding
startServer and any serverInfo assignment accordingly (the symbols to edit are
gatherSqlIntegrationEnvVars and startServer in the current method).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a755e4f-d3d3-4555-9241-2bb9ee4de1cd
📒 Files selected for processing (1)
src/kernels/deepnote/deepnoteServerStarter.node.ts
- Eliminated the port allocation serialization logic from the DeepnoteServerStarter class, as it is now handled by the @deepnote/runtime-core's startServer method. - Updated related logging messages to reflect the changes in server startup processes. - Adjusted unit tests to focus on SQL environment variable gathering and lifecycle orchestration, removing tests related to port reservation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/kernels/deepnote/deepnoteServerStarter.node.ts (2)
262-289:⚠️ Potential issue | 🟠 MajorDon't swallow cancellation in the SQL-env/startup path.
gatherSqlIntegrationEnvVars()logs and returns{}for every exception, so a canceled provider call falls through intostartServer(). After Line 266 returns, this method also never re-checkstokenbefore cachingserverInfo, attaching listeners, and writing the lock file. Re-throw cancellation in the helper, and if cancellation is requested after runtime-core starts, stop the returned server before bubbling the cancel.Also applies to: 358-387
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteServerStarter.node.ts` around lines 262 - 289, gatherSqlIntegrationEnvVars currently swallows cancellations; change it to re-throw cancellation errors (check for token.isCancellationRequested or CancellationError from provider and rethrow) so upstream knows to abort, and in the DeepnoteServerStarter flow around startServer (the block using gatherSqlIntegrationEnvVars, startServer, DeepnoteServerStartupError) add a post-start cancellation check that, if cancellation was requested, stops the started server (use serverInfo.process.pid or any server stop API) before re-throwing/propagating the cancellation; ensure you do not cache projectContext.serverInfo, call monitorServerOutput, or write the lock file (serverInfo.process.pid) when cancelled.
432-436:⚠️ Potential issue | 🟠 Major
dispose()is racing wrapper objects, not the tracked promises.
pendingOperationsstores{ type, promise }, but Line 435 passesopintoPromise.race(). Plain objects are treated as already-resolved values, sodispose()stops waiting immediately and can clear state while starts/stops are still in flight.Suggested fix
- await Promise.allSettled(pendingOps.map((op) => Promise.race([op, sleep(GRACEFUL_SHUTDOWN_TIMEOUT_MS)]))); + await Promise.allSettled( + pendingOps.map((op) => Promise.race([op.promise, sleep(GRACEFUL_SHUTDOWN_TIMEOUT_MS)])) + );Run this to verify the wrapper shape and the current
Promise.race()call. ExpectPendingOperationto expose apromisefield anddispose()to raceopinstead ofop.promise.#!/bin/bash rg -n -C2 'type PendingOperation|promise: Promise|Promise\.race' src/kernels/deepnote/deepnoteServerStarter.node.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteServerStarter.node.ts` around lines 432 - 436, dispose() is racing the wrapper objects instead of their inner promises: pendingOperations contains PendingOperation objects with a promise field, but the code calls Promise.race([op, ...]) which treats op as an already-resolved value; change the dispose() logic that builds pendingOps (and the Promise.race call) to use op.promise (e.g. Promise.race([op.promise, sleep(GRACEFUL_SHUTDOWN_TIMEOUT_MS)])) so Promise.race waits on the actual tracked Promise; update references in the dispose() function that currently use pendingOps.map(op => ...) and ensure you still handle Promise.allSettled(...) semantics with the awaited raced promises.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/kernels/deepnote/deepnoteServerStarter.node.ts`:
- Around line 118-127: The cached-server early return is skipping validation of
requested additionalPackages, so update the warm-start path in the block using
projectContexts.get(fileKey) / existingContext to also compare or reconcile
additionalPackages before returning: when existingEnvironmentId ===
environmentId and isServerRunning(existingServerInfo) is true, fetch the
requested additionalPackages from the current call context and either (a) verify
they are already installed against existingServerInfo (and only return if they
match) or (b) call installAdditionalPackages(existingServerInfo,
requestedAdditionalPackages) to install missing deps and update the stored
serverInfo in projectContexts before returning; ensure this logic lives
alongside the isServerRunning check and still returns existingServerInfo only
after verification/reconciliation.
In `@src/kernels/deepnote/deepnoteServerStarter.unit.test.ts`:
- Around line 59-77: Add a test case to cover the canceled-provider branch in
gatherSqlIntegrationEnvVars: instantiate DeepnoteServerStarter (as in the
existing test), register or mock an SQL provider that rejects with an
OperationCanceled-like error when called, call the private method
gatherSqlIntegrationEnvVars (use getPrivateMethod) with a cancelled token or
simulate cancellation, and assert that the result is {} (or that cancellation is
propagated as expected for your implementation); ensure you clean up by
disposing starter and use the same mocks (mockProcessServiceFactory,
mockToolkitInstaller, mockAgentSkillsManager, mockOutputChannel,
mockAsyncRegistry) and the Uri import to mirror the existing test structure so
the new test exercises the provider-rejection-with-cancellation path.
- Around line 80-89: The test must verify dispose() waits for in-flight
operations instead of just checking maps are empty; modify the 'dispose' test to
seed starter.pendingOperations with a deferred (create a Promise whose resolve
you control) representing a running start/stop, then call
serverStarter.dispose() asynchronously and assert it does not resolve
immediately (use a short timeout or Promise.race to ensure it's still pending),
then resolve the deferred and finally await dispose() to ensure it completes;
reference the dispose() method and the starter.pendingOperations map when adding
and removing the deferred entry and asserting behavior.
---
Duplicate comments:
In `@src/kernels/deepnote/deepnoteServerStarter.node.ts`:
- Around line 262-289: gatherSqlIntegrationEnvVars currently swallows
cancellations; change it to re-throw cancellation errors (check for
token.isCancellationRequested or CancellationError from provider and rethrow) so
upstream knows to abort, and in the DeepnoteServerStarter flow around
startServer (the block using gatherSqlIntegrationEnvVars, startServer,
DeepnoteServerStartupError) add a post-start cancellation check that, if
cancellation was requested, stops the started server (use serverInfo.process.pid
or any server stop API) before re-throwing/propagating the cancellation; ensure
you do not cache projectContext.serverInfo, call monitorServerOutput, or write
the lock file (serverInfo.process.pid) when cancelled.
- Around line 432-436: dispose() is racing the wrapper objects instead of their
inner promises: pendingOperations contains PendingOperation objects with a
promise field, but the code calls Promise.race([op, ...]) which treats op as an
already-resolved value; change the dispose() logic that builds pendingOps (and
the Promise.race call) to use op.promise (e.g. Promise.race([op.promise,
sleep(GRACEFUL_SHUTDOWN_TIMEOUT_MS)])) so Promise.race waits on the actual
tracked Promise; update references in the dispose() function that currently use
pendingOps.map(op => ...) and ensure you still handle Promise.allSettled(...)
semantics with the awaited raced promises.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 87a75357-053a-49a2-87db-46c1eed6c556
📒 Files selected for processing (2)
src/kernels/deepnote/deepnoteServerStarter.node.tssrc/kernels/deepnote/deepnoteServerStarter.unit.test.ts
…g improvements - Introduced a new `serverOutputByFile` map to track stdout and stderr outputs for each server instance, limiting the output length to improve performance and manageability. - Updated error handling in the server startup process to capture and report both stdout and stderr in case of failures, providing better diagnostics. - Adjusted the `dispose` method to ensure all internal states, including the new output tracking, are cleared appropriately. - Enhanced unit tests to validate the new output tracking functionality and ensure proper handling of cancellation errors.
- Modified the error reporting logic to ensure that stderr output is captured only when available, enhancing clarity in error messages. - This change aims to streamline the error handling process during server startup, providing more accurate feedback in case of failures.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/kernels/deepnote/deepnoteServerStarter.node.ts (3)
31-32:⚠️ Potential issue | 🟠 MajorA slow startup can escape
dispose().Line 31 allows startup to run for 120s, but Lines 455-457 wait only 3s before Lines 496-499 clear all tracking. A pending start can therefore finish after disposal and recreate listeners or lock files for an untracked server.
As per coding guidelines: "Use real cancellation tokens tied to lifecycle events instead of fake/never-cancelled tokens; create
CancellationTokenSourcetied to relevant events (e.g., notebook close, cell cancel)"Also applies to: 455-499
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteServerStarter.node.ts` around lines 31 - 32, The long fixed startup timeout (SERVER_STARTUP_TIMEOUT_MS) allows startServer to continue after dispose(), and the short GRACEFUL_SHUTDOWN_TIMEOUT_MS means servers/trackers are cleared while a startup may still be running; fix by introducing a CancellationTokenSource tied to dispose() (or the server lifecycle) and pass its token into startServer/startup logic and any async waiters so startup is aborted immediately on dispose; ensure dispose() cancels that CTS and awaits the pending startTask (or observes the cancellation) before running the cleanup that clears tracked servers/listeners (the code around dispose(), startServer, and the cleanup that removes tracked servers/listeners should be updated to use the CTS rather than relying on the 3s timeout).
103-129:⚠️ Potential issue | 🟠 MajorDon't return a warm server before honoring
additionalPackages.Lines 124-129 only check
environmentIdand liveness. A later call for the same notebook can request extra packages and still get the old server back, so those dependencies are never installed.As per coding guidelines: "Validate cached state before early returns - before returning 'already configured', verify the state is still valid (e.g., interpreter paths match, controllers aren't stale)"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteServerStarter.node.ts` around lines 103 - 129, The current early return when an existingContext matches environmentId and isServerRunning may bypass honoring requested additionalPackages; update the logic in the method that uses this.projectContexts/existingContext (check existingEnvironmentId and existingServerInfo) to validate that the server already has the requested additionalPackages installed before returning: fetch or read the server's installed package list (or store package metadata in projectContexts), compare against the passed additionalPackages array, and if any are missing, either trigger installation on the existingServerInfo (await installation) or proceed to create/configure a new server; ensure this check uses the existingServerInfo and isServerRunning helpers and only returns early when both liveness and package parity are satisfied.
264-305:⚠️ Potential issue | 🟠 MajorCancellation still turns into a successful startup.
Lines 393-395 swallow provider cancellation, and Lines 271-305 never re-check
tokenafter runtime-core returns. If the user cancels mid-start, this path can still launch, cache, and monitor a live server. Please propagate cancellation here and stop the just-started server before returning.As per coding guidelines: "Use real cancellation tokens tied to lifecycle events instead of fake/never-cancelled tokens; create
CancellationTokenSourcetied to relevant events (e.g., notebook close, cell cancel)"Also applies to: 368-397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/kernels/deepnote/deepnoteServerStarter.node.ts` around lines 264 - 305, The startup path currently ignores user cancellation: after calling startServer (and earlier gatherSqlIntegrationEnvVars) you must re-check the incoming CancellationToken (token) immediately after startServer resolves and before caching/monitoring the server; if cancellation occurred, stop the started server (use serverInfo.process.kill() or the server shutdown method on DeepnoteServerInfo), remove the entry from serverOutputByFile, avoid calling monitorServerOutput/writeLockFile or setting projectContext.serverInfo, and rethrow a cancellation error (or call token.throwIfCancellationRequested). Also replace any fake/never-cancelled tokens used in gatherSqlIntegrationEnvVars/startServer calls with a real CancellationTokenSource tied to the notebook/cell lifecycle so cancellation is propagated from the provider to this code path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/kernels/deepnote/deepnoteServerStarter.node.ts`:
- Around line 31-32: The long fixed startup timeout (SERVER_STARTUP_TIMEOUT_MS)
allows startServer to continue after dispose(), and the short
GRACEFUL_SHUTDOWN_TIMEOUT_MS means servers/trackers are cleared while a startup
may still be running; fix by introducing a CancellationTokenSource tied to
dispose() (or the server lifecycle) and pass its token into startServer/startup
logic and any async waiters so startup is aborted immediately on dispose; ensure
dispose() cancels that CTS and awaits the pending startTask (or observes the
cancellation) before running the cleanup that clears tracked servers/listeners
(the code around dispose(), startServer, and the cleanup that removes tracked
servers/listeners should be updated to use the CTS rather than relying on the 3s
timeout).
- Around line 103-129: The current early return when an existingContext matches
environmentId and isServerRunning may bypass honoring requested
additionalPackages; update the logic in the method that uses
this.projectContexts/existingContext (check existingEnvironmentId and
existingServerInfo) to validate that the server already has the requested
additionalPackages installed before returning: fetch or read the server's
installed package list (or store package metadata in projectContexts), compare
against the passed additionalPackages array, and if any are missing, either
trigger installation on the existingServerInfo (await installation) or proceed
to create/configure a new server; ensure this check uses the existingServerInfo
and isServerRunning helpers and only returns early when both liveness and
package parity are satisfied.
- Around line 264-305: The startup path currently ignores user cancellation:
after calling startServer (and earlier gatherSqlIntegrationEnvVars) you must
re-check the incoming CancellationToken (token) immediately after startServer
resolves and before caching/monitoring the server; if cancellation occurred,
stop the started server (use serverInfo.process.kill() or the server shutdown
method on DeepnoteServerInfo), remove the entry from serverOutputByFile, avoid
calling monitorServerOutput/writeLockFile or setting projectContext.serverInfo,
and rethrow a cancellation error (or call token.throwIfCancellationRequested).
Also replace any fake/never-cancelled tokens used in
gatherSqlIntegrationEnvVars/startServer calls with a real
CancellationTokenSource tied to the notebook/cell lifecycle so cancellation is
propagated from the provider to this code path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 640e39ea-fc04-4fc3-9bdb-20a134aa15c4
📒 Files selected for processing (2)
src/kernels/deepnote/deepnoteServerStarter.node.tssrc/kernels/deepnote/deepnoteServerStarter.unit.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
Chores
Refactor
Tests