Conversation
- when object property is one of the `Object.prototype` `console.diff({},{valueOf:0})`
…rty name w/ changed value more easier e.g. `myProp:`
- used to hot-fix DOM-XSS bug
- taken care by light-dark() CSS function
Signed-off-by: 🕷️ <3756473+zendive@users.noreply.github.com>
Signed-off-by: 🕷️ <3756473+zendive@users.noreply.github.com>
|
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:
📝 WalkthroughWalkthroughRefactor build tooling and Makefile (BUILD_MODE, required binaries, manifest-derived ZIP names); migrate delta HTML rendering into a new diffApi using jsondiffpatch; switch serialization to null-prototype objects; add stripDeepObjectPrototype and tests; update runtime/messaging typings and theming to CSS variables; bump manifests and various dependency pins. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Make as Makefile / Shell
participant Deno as Deno Build Script
participant Manifest as Manifest JSON
participant Pack as Zipper / Output
Dev->>Make: run make target (e.g. all, prod)
Make->>Deno: invoke BUILD_SCRIPT with BUILD_MODE, BUILD_DIR, OUTPUT_DIR
Deno->>Manifest: read manifest.chrome.json / manifest.firefox.json
Manifest-->>Deno: return CHROME_MANIFEST_VERSION / FIREFOX_MANIFEST_VERSION
Deno->>Pack: build artifacts into BUILD_DIR
Deno->>Pack: create extension.chrome-<version>.zip & extension.firefox-<version>.zip
Pack-->>Make: place zips in OUTPUT_DIR
Make-->>Dev: artifacts ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/api/diffApi.ts`:
- Around line 7-23: The objectHash function uses generic keys in
OBJECT_ID_IN_ARRAY (notably "name" and "key") which can collide across array
items; update the implementation to avoid false matches by either removing
"name" and "key" from OBJECT_ID_IN_ARRAY or enhancing objectHash to only accept
"name"/"key" when their value is unique within the array: inside objectHash
(used by patcher) iterate the parent array to verify uniqueness of the candidate
id value before returning it, otherwise continue to the next candidate or fall
back to the index; reference OBJECT_ID_IN_ARRAY and objectHash/patcher when
making the change.
- Around line 46-59: The formatted HTML produced by format(delta, left) is being
inserted directly into tmpEl.innerHTML (html variable) which can trigger XSS
warnings; import DOMPurify and call DOMPurify.sanitize on the output of
format(delta, left) and use that sanitized string when assigning tmpEl.innerHTML
(i.e., replace the direct innerHTML assignment with tmpEl.innerHTML =
DOMPurify.sanitize(htmlOrUnsafe) and ensure any variable names like html, tmpEl,
delta, left remain consistent).
In `@src/api/useRuntime.ts`:
- Around line 19-27: allListeners is typed only for synchronous callbacks
(TRuntimeListener) but connect registers async callbacks (TRuntimeListenerAsync)
and callAllListeners invokes them without awaiting, causing unhandled
rejections; fix by widening the set type to
Set<TRuntimeListener|TRuntimeListenerAsync>, make callAllListeners async and
await listener invocations (e.g., await
Promise.allSettled([...allListeners].map(l => (l(e) as Promise<void>))), or
iterate with try/catch and await each), and update any callers (e.g., connect)
to await callAllListeners so async errors are handled rather than swallowed.
🧹 Nitpick comments (8)
src/@types/index.d.ts (1)
8-9: The type ofjsdiffdoesn't reflect the actual assigned value.
wrappedJSObject.jsdiffis typed as() => void, butconsoleAPI(the value actually assigned injsdiff-console.ts) is an object withdiff,diffLeft,diffRight, anddiffPushmethods. Consider using a more accurate type (e.g.,typeof consoleAPIor an equivalent interface) so consumers get proper autocompletion and type checking.README.md (1)
150-151: Ambiguous formatting of build prerequisites.Line 151 reads as a run-on list where
makecould be interpreted as a verb rather than a tool name. Consider using inline code formatting or a bullet list to clarify each tool.Suggested fix
### Build requirements -- Linux: [deno](https://docs.deno.com/runtime/getting_started/installation/) make jq zip tree +- Linux: [deno](https://docs.deno.com/runtime/getting_started/installation/), `make`, `jq`, `zip`, `tree`src/view/panel.vue (1)
99-100:--input-background-idlehas identical light and dark values.Both sides of the
light-dark()call resolve torgba(0, 0, 0, 0.05). If intentional, this is fine but thelight-dark()wrapper is unnecessary. Same applies to--input-background-activewhere values also appear very similar.src/api/diffApi.ts (1)
4-4: Useimport typefor type-only import.
ISerializableObjectis only used as a type assertion on line 12, not as a value at runtime.Suggested fix
-import { ISerializableObject } from './clone.ts'; +import type { ISerializableObject } from './clone.ts';src/api/cloneCatalog.ts (1)
23-23: The<(Document | Element) & symbol>cast is unsound but functionally safe here.This intersection type is impossible at runtime — a value is never simultaneously a DOM node and a symbol. The cast exists to work around TypeScript's contravariant parameter narrowing when calling a union of function types (
tagcould be eitherTAG_DOM_ELEMENTorTAG_UNIQUE_SYMBOL). It works in practice because each call site inclone.tsalways pairs the correct tag with the correct key type.A type-safe alternative would be to use overloads or a generic on
lookup:♻️ Optional: type-safe approach using overloads
-type TUniqueInstanceTag = typeof TAG_DOM_ELEMENT | typeof TAG_UNIQUE_SYMBOL; +// No union type needed; use overloads instead export class UniqueLookupCatalog { `#records`: WeakMap<WeakKey, ICatalogUniqueRecord> = new WeakMap(); `#index` = 0; - lookup(key: WeakKey, tag: TUniqueInstanceTag) { + lookup(key: Document | Element, tag: typeof TAG_DOM_ELEMENT): string; + lookup(key: symbol, tag: typeof TAG_UNIQUE_SYMBOL): string; + lookup(key: WeakKey, tag: typeof TAG_DOM_ELEMENT | typeof TAG_UNIQUE_SYMBOL) { let record = this.#records.get(key); if (record) { return record; } const id = index2Id(++this.#index); - record = tag(id, <(Document | Element) & symbol> key); + record = (tag as (id: string, value: any) => string)(id, key); this.#records.set(key, record); return record; } }src/api/useRuntime.ts (1)
42-48: Firefox ports are created every 30s but never disconnected — potential resource leak.Each
setIntervaltick callsgetFirefoxPort(callAllListeners), which opens a new port and attaches the listener. The previous port is never explicitly disconnected. While Firefox MV3 ports have a limited lifespan and will eventually be garbage-collected, explicitly disconnecting the old port would be cleaner and avoid accumulating stale listeners during the overlap window.♻️ Suggested: track and disconnect the old port
if (typeof browser !== 'undefined') { // firefox - getFirefoxPort(callAllListeners); + let firefoxPort = getFirefoxPort(callAllListeners); setInterval(() => { - getFirefoxPort(callAllListeners); + firefoxPort.disconnect(); + firefoxPort = getFirefoxPort(callAllListeners); }, BACKGROUND_SCRIPT_CONNECTION_INTERVAL); } else {src/api/clone.ts (1)
311-314: Theinstanceof window.Objectbranch is redundant under normal circumstances.Any non-null value where
typeof === 'object'is already an Object instance. Theinstanceof window.Objectbranch only adds coverage for the unusual case wheretypeofreturns something other than'object'for an Object instance (cross-realm edge cases in extension content scripts). If that's intentional for Firefox content script compatibility, a brief comment would clarify the intent.Makefile (1)
61-65: Use$(MAKE)instead ofmakefor recursive invocations.Hard-coded
makebreaks when the build is invoked via a differently named binary (e.g.,gmakeon BSD systems) or when flags need to propagate.$(MAKE)is the portable way to recurse.♻️ Suggested fix
all: prod - make tune2firefox + $(MAKE) tune2firefox rm -rf $(FIREFOX_ZIP) zip -r $(FIREFOX_ZIP) $(OUTPUT_DIR) ./manifest.json > /dev/null - make tune2chrome + $(MAKE) tune2chrome rm -rf $(CHROME_ZIP) zip -r $(CHROME_ZIP) $(OUTPUT_DIR) ./manifest.json > /dev/null zip --delete $(CHROME_ZIP) "$(BUILD_DIR)firefox/*" > /dev/null
…uely distinguish objects in the array Signed-off-by: 🕷️ <3756473+zendive@users.noreply.github.com>
Signed-off-by: 🕷️ <3756473+zendive@users.noreply.github.com>
Signed-off-by: 🕷️ <3756473+zendive@users.noreply.github.com>
Signed-off-by: 🕷️ <3756473+zendive@users.noreply.github.com>
+ more readable Signed-off-by: 🕷️ <3756473+zendive@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/clone.ts (1)
248-266:⚠️ Potential issue | 🟠 Major
window.Float16Arraymay not exist in older browser versions, causinginstanceofto throw aTypeError.In browsers released before Float16Array support was added (Chrome ≤134, Firefox ≤128, Safari ≤18.1),
that instanceof window.Float16Arraywill throwTypeError: Right-hand side of instanceof is not callablerather than returningfalse.Guard with an existence check:
Suggested fix
- that instanceof window.Float16Array || + (typeof window.Float16Array === 'function' && that instanceof window.Float16Array) ||🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/clone.ts` around lines 248 - 266, The isArray function currently uses instanceof checks against several typed array constructors (e.g., window.Float16Array) which may be undefined in older browsers and cause a TypeError; update each instanceof check to first verify the constructor exists (e.g., typeof window.Float16Array !== 'undefined' or window.Float16Array != null) before using instanceof so the expression safely evaluates to false when the constructor is not present; apply this pattern for any optional globals referenced in isArray to avoid runtime exceptions.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile`:
- Around line 3-5: The Makefile currently runs REQUIRED_BINS := deno jq zip tree
and the $(foreach ...) dependency check at parse time which causes failures for
any target; change this to perform the check lazily by removing the immediate
$(foreach ...) expansion and instead run the shell existence check inside the
specific recipe(s) that need the tools (or wrap the check in a phony target like
check-deps that other targets depend on), and replace any parse-time shell
assignments (!=) used for JSON/jq extraction with simple = assignments and
invoke $(shell ...) inside the recipe or use a command-line pipeline in the
recipe so jq-based version extraction only runs when that target is executed;
reference symbols: REQUIRED_BINS, the $(foreach ...) dependency check, and any
!= assignments used for manifest/jq extraction.
In `@src/api/clone.ts`:
- Around line 248-266: The isArray function currently uses instanceof checks
against several typed array constructors (e.g., window.Float16Array) which may
be undefined in older browsers and cause a TypeError; update each instanceof
check to first verify the constructor exists (e.g., typeof window.Float16Array
!== 'undefined' or window.Float16Array != null) before using instanceof so the
expression safely evaluates to false when the constructor is not present; apply
this pattern for any optional globals referenced in isArray to avoid runtime
exceptions.
In `@src/api/diffApi.ts`:
- Around line 52-56: The call site uses cleanObjectPrototype which only strips
the prototype at the top level, so nested plain objects in delta and left may
retain prototypes and leak when formatHtml is called; implement a recursive
deepCleanObjectPrototype (or extend cleanObjectPrototype to walk nested plain
objects and remove/replace their prototype) and call that on delta and left
before passing them to formatHtml (keep the helper name referenced so you can
find the call site and replace cleanObjectPrototype(delta) /
cleanObjectPrototype(left) with deepCleanObjectPrototype(delta) /
deepCleanObjectPrototype(left)).
In `@src/api/useRuntime.ts`:
- Around line 19-22: allListeners is currently typed as Set<TRuntimeListener |
TRuntimeListenerAsync> but connect only ever adds TRuntimeListenerAsync; update
the declaration to Set<TRuntimeListenerAsync> (or, if you prefer to keep the
union, add a clarifying comment) and adjust usages: ensure connect, disconnect,
and the callAllListeners wrapper reference the narrowed type
(TRuntimeListenerAsync) and when passing a synchronous callback to browser APIs
use an explicit wrapper or cast in the call where callAllListeners is used.
Update references to allListeners, connect, and callAllListeners so the types
are consistent and the intent is documented if the union is retained.
In `@src/view/panel.vue`:
- Line 78: Replace the deprecated document.execCommand('copy') call with the
Clipboard API: use navigator.clipboard.writeText(...) to copy the text; in the
same function where document.execCommand('copy') appears (look for the literal
"document.execCommand('copy')" in panel.vue), call navigator.clipboard.writeText
with the string to copy and handle the returned Promise (then/catch or
async/await) to surface errors; because clipboard availability can be
inconsistent in DevTools contexts, add a safe fallback that attempts execCommand
only if navigator.clipboard is undefined or writeText rejects, and ensure you
log or surface any copy failures.
🧹 Nitpick comments (4)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@Makefile`: - Around line 3-5: The Makefile currently runs REQUIRED_BINS := deno jq zip tree and the $(foreach ...) dependency check at parse time which causes failures for any target; change this to perform the check lazily by removing the immediate $(foreach ...) expansion and instead run the shell existence check inside the specific recipe(s) that need the tools (or wrap the check in a phony target like check-deps that other targets depend on), and replace any parse-time shell assignments (!=) used for JSON/jq extraction with simple = assignments and invoke $(shell ...) inside the recipe or use a command-line pipeline in the recipe so jq-based version extraction only runs when that target is executed; reference symbols: REQUIRED_BINS, the $(foreach ...) dependency check, and any != assignments used for manifest/jq extraction. In `@src/api/diffApi.ts`: - Around line 52-56: The call site uses cleanObjectPrototype which only strips the prototype at the top level, so nested plain objects in delta and left may retain prototypes and leak when formatHtml is called; implement a recursive deepCleanObjectPrototype (or extend cleanObjectPrototype to walk nested plain objects and remove/replace their prototype) and call that on delta and left before passing them to formatHtml (keep the helper name referenced so you can find the call site and replace cleanObjectPrototype(delta) / cleanObjectPrototype(left) with deepCleanObjectPrototype(delta) / deepCleanObjectPrototype(left)). In `@src/api/useRuntime.ts`: - Around line 19-22: allListeners is currently typed as Set<TRuntimeListener | TRuntimeListenerAsync> but connect only ever adds TRuntimeListenerAsync; update the declaration to Set<TRuntimeListenerAsync> (or, if you prefer to keep the union, add a clarifying comment) and adjust usages: ensure connect, disconnect, and the callAllListeners wrapper reference the narrowed type (TRuntimeListenerAsync) and when passing a synchronous callback to browser APIs use an explicit wrapper or cast in the call where callAllListeners is used. Update references to allListeners, connect, and callAllListeners so the types are consistent and the intent is documented if the union is retained. In `@src/view/panel.vue`: - Line 78: Replace the deprecated document.execCommand('copy') call with the Clipboard API: use navigator.clipboard.writeText(...) to copy the text; in the same function where document.execCommand('copy') appears (look for the literal "document.execCommand('copy')" in panel.vue), call navigator.clipboard.writeText with the string to copy and handle the returned Promise (then/catch or async/await) to surface errors; because clipboard availability can be inconsistent in DevTools contexts, add a safe fallback that attempts execCommand only if navigator.clipboard is undefined or writeText rejects, and ensure you log or surface any copy failures.Makefile (1)
3-5:REQUIRED_BINScheck andjq-based version extraction run at Makefile parse time for all targets.The
$(foreach ...)on line 5 and the!=shell assignments on lines 8/11 execute when the Makefile is parsed, not when a specific target is invoked. This means evenmake cleanormake installwill fail ifjq,zip, ortreearen't yet installed, or if the manifest JSON files don't exist.If this is intentional (all tools must be present before any work), it's fine. Otherwise, consider lazy evaluation (e.g., using
=with$(shell ...)inside recipe lines, or moving checks into individual targets).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 3 - 5, The Makefile currently runs REQUIRED_BINS := deno jq zip tree and the $(foreach ...) dependency check at parse time which causes failures for any target; change this to perform the check lazily by removing the immediate $(foreach ...) expansion and instead run the shell existence check inside the specific recipe(s) that need the tools (or wrap the check in a phony target like check-deps that other targets depend on), and replace any parse-time shell assignments (!=) used for JSON/jq extraction with simple = assignments and invoke $(shell ...) inside the recipe or use a command-line pipeline in the recipe so jq-based version extraction only runs when that target is executed; reference symbols: REQUIRED_BINS, the $(foreach ...) dependency check, and any != assignments used for manifest/jq extraction.src/api/diffApi.ts (1)
52-56: Note:cleanObjectPrototypeonly strips the prototype at the top level.If
deltaorleftcontain nested plain objects with prototypes, those remain untouched. This is likely fine since objects coming fromcustomClonealready useObject.create(null)at each serialization level, but worth noting ifformatHtmlis ever called with objects not produced by the clone pipeline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/diffApi.ts` around lines 52 - 56, The call site uses cleanObjectPrototype which only strips the prototype at the top level, so nested plain objects in delta and left may retain prototypes and leak when formatHtml is called; implement a recursive deepCleanObjectPrototype (or extend cleanObjectPrototype to walk nested plain objects and remove/replace their prototype) and call that on delta and left before passing them to formatHtml (keep the helper name referenced so you can find the call site and replace cleanObjectPrototype(delta) / cleanObjectPrototype(left) with deepCleanObjectPrototype(delta) / deepCleanObjectPrototype(left)).src/view/panel.vue (1)
78-78:document.execCommand('copy')is deprecated.The Clipboard API (
navigator.clipboard.writeText()) is the modern replacement. That said, in a DevTools extension context, clipboard API availability can be inconsistent, so this may be an intentional choice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/view/panel.vue` at line 78, Replace the deprecated document.execCommand('copy') call with the Clipboard API: use navigator.clipboard.writeText(...) to copy the text; in the same function where document.execCommand('copy') appears (look for the literal "document.execCommand('copy')" in panel.vue), call navigator.clipboard.writeText with the string to copy and handle the returned Promise (then/catch or async/await) to surface errors; because clipboard availability can be inconsistent in DevTools contexts, add a safe fallback that attempts execCommand only if navigator.clipboard is undefined or writeText rejects, and ensure you log or surface any copy failures.src/api/useRuntime.ts (1)
19-22: Minor type mismatch:connectonly acceptsTRuntimeListenerAsyncbutallListenersincludesTRuntimeListener.
allListenersis typed asSet<TRuntimeListener | TRuntimeListenerAsync>but onlyTRuntimeListenerAsynclisteners are ever added viaconnect(). TheTRuntimeListenervariant in the union is only there becausecallAllListeners(which is sync) is used as a direct callback for browser APIs. This works, but the union type on the set is a bit misleading—consider a clarifying comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/useRuntime.ts` around lines 19 - 22, allListeners is currently typed as Set<TRuntimeListener | TRuntimeListenerAsync> but connect only ever adds TRuntimeListenerAsync; update the declaration to Set<TRuntimeListenerAsync> (or, if you prefer to keep the union, add a clarifying comment) and adjust usages: ensure connect, disconnect, and the callAllListeners wrapper reference the narrowed type (TRuntimeListenerAsync) and when passing a synchronous callback to browser APIs use an explicit wrapper or cast in the call where callAllListeners is used. Update references to allListeners, connect, and callAllListeners so the types are consistent and the intent is documented if the union is retained.
Signed-off-by: 🕷️ <3756473+zendive@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/api/clone.ts (1)
311-313:isObjectreturnstrueforMap,Set,Date,RegExp, and DOM nodes — the name and type predicate are broader than "plain object".Within
recursiveClonethe call-ordering protects against false positives, but as an exported API the signaturethat is ISerializableObjectwith no exclusion of well-known non-plain types is easy to misuse (seecleanObjectPrototypebelow).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/clone.ts` around lines 311 - 313, isObject is too permissive: it returns true for Map/Set/Date/RegExp/DOM nodes but is exported as "ISerializableObject", which risks misuse in callers like recursiveClone and cleanObjectPrototype; change the implementation of isObject to detect plain objects only (e.g., ensure that that !== null && typeof that === 'object' && Object.prototype.toString.call(that) === '[object Object]' OR Object.getPrototypeOf(that) === Object.prototype || Object.getPrototypeOf(that) === null) so Maps/Sets/Dates/Regexps/DOM nodes are excluded, and keep the type predicate as ISerializableObject (or adjust the name if you prefer) to reflect the narrower check; update any usages that rely on the old broad behavior (notably recursiveClone and cleanObjectPrototype) if necessary.
🤖 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/api/clone.ts`:
- Around line 323-329: cleanObjectPrototype currently treats any non-null object
(per isObject) as a plain object and therefore discards contents of
Map/Set/Date/class instances; change the guard so only true plain objects are
converted by checking the object's direct prototype via
Object.getPrototypeOf(unk) and only proceed when proto === Object.prototype or
proto === null (preserving prototype-less objects), otherwise return unk
unchanged; update the logic around cleanObjectPrototype (and reuse
isArray/isObject checks) to avoid converting Map/Set/Date or class instances.
---
Nitpick comments:
In `@src/api/clone.ts`:
- Around line 311-313: isObject is too permissive: it returns true for
Map/Set/Date/RegExp/DOM nodes but is exported as "ISerializableObject", which
risks misuse in callers like recursiveClone and cleanObjectPrototype; change the
implementation of isObject to detect plain objects only (e.g., ensure that that
!== null && typeof that === 'object' && Object.prototype.toString.call(that) ===
'[object Object]' OR Object.getPrototypeOf(that) === Object.prototype ||
Object.getPrototypeOf(that) === null) so Maps/Sets/Dates/Regexps/DOM nodes are
excluded, and keep the type predicate as ISerializableObject (or adjust the name
if you prefer) to reflect the narrower check; update any usages that rely on the
old broad behavior (notably recursiveClone and cleanObjectPrototype) if
necessary.
Signed-off-by: 🕷️ <3756473+zendive@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/utils.test.ts (1)
16-19: Consider adding an assertion thattrimmed.arris still anArray.The function intentionally preserves arrays (only plain objects lose their prototype). A
toBeInstanceOf(Array)check ontrimmed.arrwould document this design contract and catch any future regression where arrays are also stripped:♻️ Optional addition
expect(trimmed).not.toBeInstanceOf(Object); expect(trimmed.obj).not.toBeInstanceOf(Object); + expect(trimmed.arr).toBeInstanceOf(Array); expect(trimmed.arr[0]).not.toBeInstanceOf(Object); expect(trimmed.arr[0].obj).not.toBeInstanceOf(Object);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/utils.test.ts` around lines 16 - 19, Add an assertion to verify arrays are preserved by the trimming behavior: in tests/utils.test.ts add a check that trimmed.arr is still an Array (e.g. expect(trimmed.arr).toBeInstanceOf(Array)) alongside the existing assertions (trimmed, trimmed.obj, trimmed.arr[0], trimmed.arr[0].obj) to document and guard the contract that arrays keep their prototype.src/api/diffApi.ts (1)
41-43:formatDeltaAsRFC6902is unguarded unlikebuildDeltaElement.
buildDeltaElementwraps formatter calls in a try/catch, butformatDeltaAsRFC6902lets errors fromformatRFC6902propagate uncaught to callers. If the caller isn't prepared for a throw, this could produce an unhandled rejection. Consider either documenting that callers must handle errors, or wrapping consistently:♻️ Suggested hardening
export function formatDeltaAsRFC6902(delta: Delta) { - return formatRFC6902(delta); + try { + return formatRFC6902(delta); + } catch (e) { + console.error('formatDeltaAsRFC6902', e); + return undefined; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/diffApi.ts` around lines 41 - 43, formatDeltaAsRFC6902 currently calls formatRFC6902 without protection and can propagate throws; wrap the call in a try/catch (mirroring buildDeltaElement) so failures are handled consistently—call formatRFC6902(delta) inside try, return the result on success, and in catch either throw a new Error with contextual message that includes the original error (or return a safe sentinel like null if your codebase prefers) so callers won't receive unhandled exceptions; reference function names formatDeltaAsRFC6902, formatRFC6902, and buildDeltaElement to locate and align behavior.
🤖 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/api/clone.ts`:
- Around line 327-350: stripDeepObjectPrototype currently mutates arrays
in-place while creating a fresh prototype-less object for object inputs, causing
asymmetric behavior and potential state corruption; update the array branch in
stripDeepObjectPrototype to allocate and return a new array (e.g., build a new
array and push or map each element through stripDeepObjectPrototype) instead of
assigning into unk[n], ensuring you preserve typings and still recursively
process elements so that arrays are not mutated in-place while objects continue
to return Object.create(null).
In `@src/api/diffApi.ts`:
- Around line 53-56: stripDeepObjectPrototype currently mutates arrays in-place
causing callers like the use in formatHtml(stripDeepObjectPrototype(delta), ...)
to unexpectedly alter referenced arrays; modify stripDeepObjectPrototype (the
array branch) to allocate and return a new array instead of rewriting the
original: iterate/map over each element, recursively call
stripDeepObjectPrototype on elements, push results into a new array, and return
that new array (mirror how the object branch creates Object.create(null));
ensure no in-place assignments occur so callers' original array references
remain unchanged.
In `@tests/utils.test.ts`:
- Around line 13-15: The test currently calls stripDeepObjectPrototype(obj) and
then compares trimmed to obj, but stripDeepObjectPrototype mutates arrays
in-place so obj is already changed; before calling stripDeepObjectPrototype
capture the original expected structure (e.g., deep clone or structured clone of
obj) and then call stripDeepObjectPrototype, and finally assert
expect(trimmed).toEqual(expected) instead of comparing against the mutated obj;
reference stripDeepObjectPrototype, trimmed and obj to locate the test and
replace the assertion.
---
Nitpick comments:
In `@src/api/diffApi.ts`:
- Around line 41-43: formatDeltaAsRFC6902 currently calls formatRFC6902 without
protection and can propagate throws; wrap the call in a try/catch (mirroring
buildDeltaElement) so failures are handled consistently—call
formatRFC6902(delta) inside try, return the result on success, and in catch
either throw a new Error with contextual message that includes the original
error (or return a safe sentinel like null if your codebase prefers) so callers
won't receive unhandled exceptions; reference function names
formatDeltaAsRFC6902, formatRFC6902, and buildDeltaElement to locate and align
behavior.
In `@tests/utils.test.ts`:
- Around line 16-19: Add an assertion to verify arrays are preserved by the
trimming behavior: in tests/utils.test.ts add a check that trimmed.arr is still
an Array (e.g. expect(trimmed.arr).toBeInstanceOf(Array)) alongside the existing
assertions (trimmed, trimmed.obj, trimmed.arr[0], trimmed.arr[0].obj) to
document and guard the contract that arrays keep their prototype.
| export function stripDeepObjectPrototype<T>(unk: T) { | ||
| if (Object.prototype.toString.call(unk) === '[object Array]') { | ||
| // @ts-expect-error in 2026, typescript doesn't know yet | ||
| for (let n = 0; n < unk.length; n++) { | ||
| // @ts-expect-error in 2026, typescript doesn't know yet | ||
| unk[n] = stripDeepObjectPrototype(unk[n]); | ||
| } | ||
|
|
||
| return unk; | ||
| } | ||
|
|
||
| if (Object.prototype.toString.call(unk) === '[object Object]') { | ||
| const rv = Object.create(null); | ||
| const obj = <ISerializableObject> unk; | ||
|
|
||
| for (const key of Reflect.ownKeys(obj)) { | ||
| rv[key] = stripDeepObjectPrototype(obj[key]); | ||
| } | ||
|
|
||
| return <T> rv; | ||
| } | ||
|
|
||
| return unk; | ||
| } |
There was a problem hiding this comment.
stripDeepObjectPrototype mutates arrays in-place — asymmetric with the object branch.
The array branch (lines 328–336) rewrites unk[n] on the original array, returning the same reference. The object branch (lines 338–347) allocates a fresh Object.create(null). This inconsistency means callers passing an object whose properties hold array values will have those arrays silently mutated, corrupting any other holder of those references (e.g., reactive store state). See the companion comment in src/api/diffApi.ts (lines 53–56) for the direct impact. The fix is to return a new array from the array branch instead of rewriting in-place (proposed diff in the diffApi.ts comment).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/clone.ts` around lines 327 - 350, stripDeepObjectPrototype currently
mutates arrays in-place while creating a fresh prototype-less object for object
inputs, causing asymmetric behavior and potential state corruption; update the
array branch in stripDeepObjectPrototype to allocate and return a new array
(e.g., build a new array and push or map each element through
stripDeepObjectPrototype) instead of assigning into unk[n], ensuring you
preserve typings and still recursively process elements so that arrays are not
mutated in-place while objects continue to return Object.create(null).
| html = formatHtml( | ||
| stripDeepObjectPrototype(delta), | ||
| stripDeepObjectPrototype(left), | ||
| ); |
There was a problem hiding this comment.
stripDeepObjectPrototype silently mutates any arrays nested within delta and left.
stripDeepObjectPrototype for the array branch rewrites elements in-place on the original array reference rather than copying it (see clone.ts lines 329–332). A jsondiffpatch delta is a plain object whose property values are typically arrays such as [oldVal, newVal]. Calling stripDeepObjectPrototype(delta) creates a new null-prototype object for the top level, but the inner [oldVal, newVal] arrays are mutated in-place. Any other holder of those array references — e.g., a Pinia store, a cached delta, or the original left object's own arrays — will silently see their elements replaced with null-prototype objects after this call.
The safest fix is to change the array branch in stripDeepObjectPrototype to return a new array instead of mutating in-place, consistent with how the object branch already allocates a fresh Object.create(null):
🛡️ Proposed fix in src/api/clone.ts
export function stripDeepObjectPrototype<T>(unk: T) {
if (Object.prototype.toString.call(unk) === '[object Array]') {
- // `@ts-expect-error` in 2026, typescript doesn't know yet
- for (let n = 0; n < unk.length; n++) {
- // `@ts-expect-error` in 2026, typescript doesn't know yet
- unk[n] = stripDeepObjectPrototype(unk[n]);
- }
-
- return unk;
+ // `@ts-expect-error` in 2026, typescript doesn't know yet
+ const rv: unknown[] = (unk as unknown[]).map((item) =>
+ stripDeepObjectPrototype(item)
+ );
+ return rv as unknown as T;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/diffApi.ts` around lines 53 - 56, stripDeepObjectPrototype currently
mutates arrays in-place causing callers like the use in
formatHtml(stripDeepObjectPrototype(delta), ...) to unexpectedly alter
referenced arrays; modify stripDeepObjectPrototype (the array branch) to
allocate and return a new array instead of rewriting the original: iterate/map
over each element, recursively call stripDeepObjectPrototype on elements, push
results into a new array, and return that new array (mirror how the object
branch creates Object.create(null)); ensure no in-place assignments occur so
callers' original array references remain unchanged.
| const trimmed = stripDeepObjectPrototype(obj); | ||
|
|
||
| expect(trimmed).toEqual(obj); |
There was a problem hiding this comment.
toEqual(obj) compares against the already-mutated original.
Because the array branch of stripDeepObjectPrototype rewrites elements in-place, obj.arr is the same array reference as trimmed.arr, and obj.arr[0] has already been replaced with a null-prototype object by the time Line 15 runs. The toEqual assertion therefore compares trimmed against a partially mutated obj, not the original value — it succeeds trivially but does not validate that the output matches the original structure.
Capture the expected shape before the call to make the assertion meaningful:
♻️ Suggested fix
+ const expected = {
+ obj: {},
+ arr: [{ obj: {} }],
+ };
const trimmed = stripDeepObjectPrototype(obj);
- expect(trimmed).toEqual(obj);
+ expect(trimmed).toEqual(expected);📝 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 trimmed = stripDeepObjectPrototype(obj); | |
| expect(trimmed).toEqual(obj); | |
| const expected = { | |
| obj: {}, | |
| arr: [{ obj: {} }], | |
| }; | |
| const trimmed = stripDeepObjectPrototype(obj); | |
| expect(trimmed).toEqual(expected); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/utils.test.ts` around lines 13 - 15, The test currently calls
stripDeepObjectPrototype(obj) and then compares trimmed to obj, but
stripDeepObjectPrototype mutates arrays in-place so obj is already changed;
before calling stripDeepObjectPrototype capture the original expected structure
(e.g., deep clone or structured clone of obj) and then call
stripDeepObjectPrototype, and finally assert expect(trimmed).toEqual(expected)
instead of comparing against the mutated obj; reference
stripDeepObjectPrototype, trimmed and obj to locate the test and replace the
assertion.
1 participant
Object.create(null)instead of just{}for clean object instantiation.Summary by CodeRabbit
New Features
Removed Features
Version Updates
Chores
Tests