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 formatter into a new diffApi using jsondiffpatch; switch serialization to null-prototype objects and add stripDeepObjectPrototype with tests; update runtime typing to async listeners; bump manifests and several deps. 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->>Make: validate REQUIRED_BINS
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 (2 warnings)
✅ 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.
Signed-off-by: 🕷️ <3756473+zendive@users.noreply.github.com>
+ get rid of `__v_skip`, coming from vue@3.5.28 Signed-off-by: 🕷️ <3756473+zendive@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/api/diffApi.ts (1)
41-47:formatDeltaAsRFC6902has an implicitOperation[] | stringunion return type.On success it returns
formatRFC6902(delta)(anOperation[]); on error it returnsString(e). Without an explicit annotation callers can't statically distinguish the two.♻️ Suggested explicit return type
-export function formatDeltaAsRFC6902(delta: Delta) { +export function formatDeltaAsRFC6902(delta: Delta): ReturnType<typeof formatRFC6902> | string {🤖 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 - 47, The function formatDeltaAsRFC6902 currently returns either formatRFC6902(delta) (Operation[]) or String(e) (string) implicitly; add an explicit return type to make this union clear (e.g., annotate as Operation[] | string on formatDeltaAsRFC6902) and keep the existing try/catch behavior (ensure you reference formatDeltaAsRFC6902, formatRFC6902 and the Delta type when updating the signature); optionally improve the catch to return a clearer error string but the primary change is adding the explicit return type annotation.
🤖 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/stores/compare.store.ts`:
- Around line 24-30: The code calls markRaw on a frozen object which can throw
when Vue tries to define __v_skip; in castrateObject you should first create the
null-prototype object via stripDeepObjectPrototype(that), call markRaw on that
mutable object (so markRaw can set __v_skip), then freeze the result with
Object.freeze before returning; update castrateObject to avoid calling
markRaw(Object.freeze(...)) and instead markRaw(...) then Object.freeze(...)
while still returning the frozen, raw-marked object.
---
Duplicate comments:
In `@src/api/diffApi.ts`:
- Around line 67-68: The linter flags virtualEl.innerHTML assignment as unsafe
but the html variable is already sanitized via DOMPurify.sanitize; fix by making
the sanitization explicit and clear to static analysis: either assign the
sanitized result to a new variable (e.g., sanitizedHtml =
DOMPurify.sanitize(html || '')) and use sanitizedHtml when setting
virtualEl.innerHTML, or annotate/suppress the specific rule
(dom-content-modification / unsafe-html-content-assignment) immediately before
the innerHTML assignment so the analyzer recognizes the value is safe; reference
the DOMPurify.sanitize call and the virtualEl.innerHTML assignment when making
the change.
---
Nitpick comments:
In `@src/api/diffApi.ts`:
- Around line 41-47: The function formatDeltaAsRFC6902 currently returns either
formatRFC6902(delta) (Operation[]) or String(e) (string) implicitly; add an
explicit return type to make this union clear (e.g., annotate as Operation[] |
string on formatDeltaAsRFC6902) and keep the existing try/catch behavior (ensure
you reference formatDeltaAsRFC6902, formatRFC6902 and the Delta type when
updating the signature); optionally improve the catch to return a clearer error
string but the primary change is adding the explicit return type annotation.
Signed-off-by: 🕷️ <3756473+zendive@users.noreply.github.com>
1 participant
Object.create(null)instead of just{}for clean object instantiation.Summary by CodeRabbit
New Features
Removed Features
Version Updates
Chores
Tests