testing

[gurpreet319]

No description provided.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate timer-wrapper issues remain.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates the JSDOM compatibility layer with enhanced timer wrappers and callback invocation support.

Changes:

• Adds reusable timer and clear-timer wrappers.
• Forwards callback arguments and logs callback errors.
• Supports .call, .apply, and .bind.

File summaries

File	Summary and findings
src/jsc/modules/linkedjsdomwrapper.js	Implements timer wrappers. Critical (1 vote): captures undefined JSDOM timer properties. Moderate (1 vote): removes empty-ID guards, causing spurious warnings.

Review details

Suppressed comments (3)

src/jsc/modules/linkedjsdomwrapper.js:292

• This catch logs but swallows exceptions from timer callbacks. For setInterval, the native dispatch therefore sees a successful wrapper and reschedules it, so a throwing callback repeats indefinitely and is no longer reported through normal JavaScript exception handling. Re-throw after logging (or otherwise cancel the repeating timer) to preserve callback failure semantics.

					} catch (e) {

						console.error(
							"Timer callback error:",
							e

src/jsc/modules/linkedjsdomwrapper.js:318

• Function.prototype.apply supports an arbitrary number of timer arguments, and the native binding already forwards all arguments after the callback and delay. This new branch makes calls with more than five total arguments throw synchronously, so valid callbacks with additional parameters cannot be scheduled. Delegate to the original Function.prototype.apply instead of imposing this limit.

					default:
						throw new Error(
							"setTimeout/setInterval.apply: too many arguments"
						);
				}

src/jsc/modules/linkedjsdomwrapper.js:354

• The old clear wrappers skipped null/undefined IDs, but all four paths here now call the native binding directly. That binding logs a warning for an empty ID (src/jsc/JavaScriptUtils.cpp:476-482), and node-fetch calls clearTimeout(reqTimeout) even when no request timeout was configured (src/jsc/modules/node-fetch.js:2574-2577), so ordinary failed requests will emit spurious warning logs. Restore the empty-ID guard and route apply/call/bind through the guarded wrapper.

			var wrapper = function (id) {
				return nativeFn(id);
			};

			wrapper.apply = function (thisArg, args) {

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

Four moderate issues must be addressed before approval.

Review details

Suppressed comments (4)

Previously missed (2) — in code that hasn't changed since the last review.

src/jsc/modules/linkedjsdomwrapper.js:280

• createTimerWrapper hides the native argument validation by always passing its own closure to nativeFn. As a result, setTimeout(null, ...), setInterval(undefined, ...), and similar calls return a timer ID and silently no-op in invokeCallback, whereas the native binding rejects non-function callbacks (JavaScriptUtils.cpp:422-429). Validate the callback before scheduling so invalid inputs do not appear to succeed.
  src/jsc/modules/linkedjsdomwrapper.js:317
• This artificial five-argument limit makes valid timer calls fail: the native binding explicitly accepts every argument after the delay and the wrapper itself stores arbitrary callback arguments. wrapper.call and wrapper.bind also reach this branch through wrapper.apply, so callers passing six or more total timer arguments get a synchronous exception.

src/jsc/modules/linkedjsdomwrapper.js:366

• The returned bound clear function discards the arguments bound after thisArg. Therefore clearTimeout.bind(null, timerId)() (and the equivalent clearInterval call) invokes nativeFn(undefined) instead of clearing timerId. Capture the bound arguments and pass them through when the returned function is called.

			wrapper.bind = function () {
				return function (id) {
					return nativeFn(id);
				};
			};

src/jsc/modules/linkedjsdomwrapper.js:375

• This IIFE is invoked as (window), so these assignments only replace the timers on the JSDOM window; the bare setTimeout/setInterval/clear functions installed on the execution global remain the native bindings. Consequently, callers using setTimeout.apply/call/bind or the new callback/error behavior through the global APIs do not receive this wrapper, unlike the old implementation. Install the same wrapped functions on globalThis as well as on window.

		global.setTimeout =
			createTimerWrapper(nativeSetTimeout);

		global.setInterval =
			createTimerWrapper(nativeSetInterval);

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved timer-wrapper defects and the unrelated generated artifact must be addressed.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 4
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved timer-wrapper defects and an unrelated history dump must be addressed.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/jsc/modules/linkedjsdomwrapper.js:225

• These values are read from window, but in the full JSDOM path the native timer bindings are installed on the execution global (JavaScriptContext::add), while the JSDOM window object has no timer properties before this IIFE. Consequently all newly installed wrappers call undefined for setTimeout/setInterval (and the clear functions), so any timer used after this file throws. Capture the native functions from the execution-global bindings before replacing them.

    var nativeSetTimeout = global.setTimeout;
    var nativeClearTimeout = global.clearTimeout;
    var nativeSetInterval = global.setInterval;
    var nativeClearInterval = global.clearInterval;

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Three unresolved moderate timer-wrapper issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/jsc/modules/linkedjsdomwrapper.js:225

• utils.js initializes global as a separate empty object (utils/utils.js:20), while the native timer bindings are installed on the execution global in JavaScriptContext::registerUtils (src/jsc/JavaScriptContext.cpp:408-411). Consequently all four values captured here are undefined, and every wrapped timer later throws when it calls nativeFn(...); capture the native functions from the execution global (or the bare timer names) instead of global/the JSDOM window.

    var nativeSetTimeout = global.setTimeout;
    var nativeClearTimeout = global.clearTimeout;
    var nativeSetInterval = global.setInterval;
    var nativeClearInterval = global.clearInterval;

src/jsc/modules/linkedjsdomwrapper.js:328

• The captured timer bindings are rtFunctionWrapper host-callable objects, not ordinary JavaScript functions (src/jsc/JavaScriptWrapper.cpp:195-223), so they do not provide the .apply method this call assumes. As a result, clearTimeout.bind(...)() and clearInterval.bind(...)() throw instead of clearing the timer; invoke the native clear function directly with the first bound argument (the native API only consumes one id).

                return nativeFn.apply(
                    null,
                    args.length ? args : [undefined]
                );

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite