Fix magento #1229
Fix magento #1229
Conversation
Magento cart fix
feat(magento): cart fallback
Tagging OptionsShould a new tag be published when this PR is merged?
|
📝 WalkthroughWalkthroughRefactors GraphQL client request construction and method selection; adds richer runtime pageview/event tracker; introduces a loader to extract pathnames from requests and wires it into the manifest; adjusts route ranking and URLPattern construction; and adds a Magento image-optimizer branch. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant GraphQLClient as createGraphqlClient
participant MethodHelper as getMethodAndProps
participant ParamsHelper as getParamsAndBody
participant HTTP
Client->>GraphQLClient: query(GraphQLQueryProps, DecoRequestInit)
GraphQLClient->>MethodHelper: determine method & props
MethodHelper-->>GraphQLClient: { method, props }
alt GET
GraphQLClient->>ParamsHelper: build searchParams (minify, stringify)
ParamsHelper-->>GraphQLClient: searchParams
GraphQLClient->>HTTP: GET with searchParams
else POST
GraphQLClient->>ParamsHelper: build body (query, variables, opName)
ParamsHelper-->>GraphQLClient: body
GraphQLClient->>HTTP: POST with body
end
HTTP-->>Client: response
sequenceDiagram
autonumber
participant Browser
participant TrackerScript
participant DecoEvents
participant Analytics as window.stonks
Browser->>TrackerScript: page load / pushState / popstate
TrackerScript->>TrackerScript: capture URL, resolve path, parse UTM, truncate payload
DecoEvents->>TrackerScript: deco:* events (flags, props)
TrackerScript->>TrackerScript: aggregate props & identifiers
TrackerScript->>Analytics: stonks.pageView(payload)
DecoEvents->>TrackerScript: named events
TrackerScript->>Analytics: stonks.event(name, serializedValues)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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
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: 5
🧹 Nitpick comments (4)
website/components/OneDollarStats.tsx (1)
30-33: Exporting full tracker script risks bundle bloat and config drift.Consider moving
trackerOriginalto a separate asset/module or guarding behind dev-only export; it hardcodeshttps://collector.onedollarstats.com/eventswhile the component usesDEFAULT_COLLECTOR_ADDRESS(https://d.lilstts.com/events). This can confuse consumers if imported.utils/graphql.ts (3)
159-162:stringfyVariablesreturns"undefined"for empty input; rename and make optional.Return
undefinedwhen no variables to keep URLs short, and fix spelling.Apply:
-const stringfyVariables = <V>({ - variables, -}: Pick<GraphQLQueryProps<V>, "variables">) => JSON.stringify(variables); +const stringifyVariables = <V>({ + variables, +}: Pick<GraphQLQueryProps<V>, "variables">) => + variables === undefined ? undefined : JSON.stringify(variables);And update callers:
- const stringfiedVariables = stringfyVariables<V>({ variables }); + const stringfiedVariables = stringifyVariables<V>({ variables });
163-179: UseencodeURIComponentand includeoperationNameconsistently.For length estimation, prefer
encodeURIComponentand only appendoperationNamewhen present to avoid inflating length.- const urlLength = `${href}?query=${encodeURI(query)}&variables=${ - encodeURI( - variables, - ) - }&operationname=${operationName}`; + const qp = [ + `query=${encodeURIComponent(query)}`, + `variables=${encodeURIComponent(variables ?? "")}`, + operationName ? `operationName=${encodeURIComponent(operationName)}` : "", + ].filter(Boolean).join("&"); + const urlLength = `${href}?${qp}`;
84-124: Guard against GET with hugefragmentsarrays.If
fragmentsis large,isPostMethodRequiredmay still choose GET before minification is applied to all parts. Consider estimating with the joined+minified query to avoid overly long URLs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
utils/graphql.ts(4 hunks)website/components/OneDollarStats.tsx(1 hunks)website/functions/requestToPathname.ts(1 hunks)website/handlers/router.ts(3 hunks)website/manifest.gen.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
website/functions/requestToPathname.ts (1)
website/functions/requestToParam.ts (1)
RequestURLParam(3-3)
utils/graphql.ts (1)
utils/fetch.ts (1)
DecoRequestInit(34-36)
🔇 Additional comments (4)
website/manifest.gen.ts (1)
15-16: Manifest wiring looks correct.Import and mapping for
website/functions/requestToPathname.tsadded alongsiderequestToParam. No action needed.Also applies to: 64-65
utils/graphql.ts (2)
16-26: Type change to optionalbodyin GraphQLAPI is fine.This aligns with GET support; no issues spotted.
137-151: Minifier correctly handles nested selections; no changes required.The test confirms your minifier produces correct output for nested GraphQL selections. The outer braces
{ checkout(...) { ...Checkout } }are not matched by the first regex (which explicitly requires[^{}]*—no nested braces), but the subsequent whitespace-stripping replacements handle nesting correctly. The minified output is valid:query GetCart($checkoutId:String!){checkout(checkoutId:$checkoutId){...Checkout}}. No fix or test addition is necessary.website/handlers/router.ts (1)
88-99: I need to verify the URLPattern API specification to understand how thesearchproperty should be formatted.Let me search for more specific information about how URLPattern handles the search property when constructed with a base URL.
Let me search for more specific information about what the
.searchproperty returns when a URLPattern is constructed from a URL string without a query component.Based on my verification, I can now provide the final response.
No issues found with the URLPattern search component formatting.
The URLPattern search property is a string containing the pattern used to match the search part of a URL, and is the normalized value of the search pattern passed to the constructor or the default value ("*"), which matches any search part. When using search in a URLPattern constructor object, the value does not include the leading question mark—for example,
const pattern = new URLPattern({ search: "q=baby" })logspattern.searchas"q=baby".The code correctly handles both cases: when
patternWithDefaultOrigin.searchis an empty string (from a URL with no query component), the fallback to"*"maintains consistent formatting since neither value includes a leading?.
| return { | ||
| query: async <D, V>( | ||
| { query = "", fragments = [], variables, operationName }: { | ||
| query: string; | ||
| fragments?: string[]; | ||
| variables?: V; | ||
| operationName?: string; | ||
| }, | ||
| init?: RequestInit, | ||
| { | ||
| query = "", | ||
| fragments = [], | ||
| variables, | ||
| operationName, | ||
| }: GraphQLQueryProps<V>, | ||
| init?: DecoRequestInit, | ||
| ): Promise<D> => { | ||
| const { data, errors } = await http[key as any]({}, { | ||
| ...init, | ||
| body: { | ||
| query: [query, ...fragments].join("\n"), | ||
| variables: variables as any, | ||
| operationName, | ||
| }, | ||
| const { key, props } = getMethodAndProps<V>({ | ||
| query, | ||
| fragments, | ||
| variables, | ||
| operationName, | ||
| url, | ||
| }); | ||
| const { searchParams, body } = getParamsAndBody({ key, props, init }); | ||
| const { data, errors } = await http[key as any](searchParams, { | ||
| ...body, | ||
| }).then((res) => res.json()); | ||
|
|
There was a problem hiding this comment.
POST body may not be JSON‑stringified; init.body can override payload.
getParamsAndBodyreturns{ body: props, ...init }for POST; ifcreateHttpClientdoesn’t stringify, requests may send[object Object]or fail.- Spreading
...initafterbody: propscan replace the computed body.
Ensure JSON string body and keep props as the final body.
Apply:
- const { searchParams, body } = getParamsAndBody({ key, props, init });
- const { data, errors } = await http[key as any](searchParams, {
- ...body,
- }).then((res) => res.json());
+ const { searchParams, init: httpInit } = getParamsAndBody({ key, props, init });
+ const { data, errors } = await http[key as any](searchParams, httpInit)
+ .then((res) => res.json());And update helper (see next comment).
Committable suggestion skipped: line range outside the PR's diff.
| const getParamsAndBody = ({ | ||
| key, | ||
| props, | ||
| init, | ||
| }: ReturnType<typeof getMethodAndProps> & { init?: DecoRequestInit }) => { | ||
| if (key.startsWith("POST")) { | ||
| return { searchParams: {}, body: { body: props, ...init } }; | ||
| } | ||
| return { searchParams: { ...props }, body: { ...init } }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Harden GET/POST param packing and avoid undefined values.
- For POST, JSON‑stringify the payload and ensure it wins over
init.body. - For GET, omit keys with
undefinedto avoidvariables=undefinedin the URL.
Apply:
-const getParamsAndBody = ({
+const getParamsAndBody = ({
key,
props,
init,
}: ReturnType<typeof getMethodAndProps> & { init?: DecoRequestInit }) => {
if (key.startsWith("POST")) {
- return { searchParams: {}, body: { body: props, ...init } };
+ const bodyJson = JSON.stringify(props);
+ return { searchParams: {}, init: { ...(init ?? {}), body: bodyJson } };
}
- return { searchParams: { ...props }, body: { ...init } };
+ const searchParams: Record<string, string> = {};
+ if (props.query) searchParams.query = props.query;
+ if (props.operationName) searchParams.operationName = props.operationName;
+ if (typeof props.variables === "string" && props.variables.length > 0) {
+ searchParams.variables = props.variables;
+ }
+ return { searchParams, init: { ...(init ?? {}) } };
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In utils/graphql.ts around lines 126 to 135, harden the GET/POST param packing
by: for POST requests ensure the request body is a JSON string of props
(JSON.stringify(props)) and that this serialized payload takes precedence over
any init.body (i.e., set body: { body: JSON.stringify(props), ...init } but
ensure init.body is not used if props provided); for GET requests build
searchParams by copying props but filtering out keys whose values are undefined
so you don't produce query entries like variables=undefined (omit
undefined-valued keys before returning searchParams); keep the same return shape
({ searchParams, body }) and preserve other init fields.
| export interface Props { | ||
| /** | ||
| * @description Path name to remove from the URL - Stringfied regex. Example: "/product/" transform "/product/my-sku" in "my-sku". "/product/|/store" transform "/product/my-sku/store" in "my-sku" | ||
| * @format dynamic-options | ||
| */ | ||
| pathname?: string; | ||
| } |
There was a problem hiding this comment.
Doc nit: typo and clarify examples.
“Stringfied” → “Stringified”. Consider clarifying anchoring (prefix-only vs anywhere) in examples to avoid accidental middle replacements.
🤖 Prompt for AI Agents
In website/functions/requestToPathname.ts around lines 4 to 10, fix the
documentation typo ("Stringfied" → "Stringified") and clarify the examples to
state whether the provided string is treated as a full/anchored pattern or can
match anywhere; update the examples to explicitly show anchoring behavior (e.g.,
prefix-only vs anywhere) and mention that the value is a stringified regex so
callers should include anchors (or that the code will add anchors) to avoid
accidental middle replacements.
| const requestToParam: LoaderFunction< | ||
| Props, | ||
| RequestURLParam, | ||
| FunctionContext | ||
| > = (req, ctx) => { | ||
| const url = new URL(req.url); | ||
| const regex = new RegExp("(" + ctx.state.$live.pathname + ")", "g"); | ||
| return { | ||
| data: url.pathname.toString().replace(regex, ""), | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Props ignored and regex can over‑match; anchor and escape.
Props.pathnameis never used; current code relies onctx.state.$live.pathname.- The regex isn’t anchored and may strip matches anywhere in the path; unescaped input can behave unexpectedly.
Use props?.pathname ?? ctx.state.$live.pathname, anchor to start, and fall back safely.
Apply:
-const requestToParam: LoaderFunction<
- Props,
- RequestURLParam,
- FunctionContext
-> = (req, ctx) => {
- const url = new URL(req.url);
- const regex = new RegExp("(" + ctx.state.$live.pathname + ")", "g");
- return {
- data: url.pathname.toString().replace(regex, ""),
- };
-};
+const requestToParam: LoaderFunction<
+ Props,
+ RequestURLParam,
+ FunctionContext
+> = (req, ctx, props) => {
+ const url = new URL(req.url);
+ const pattern = (props?.pathname ?? ctx.state.$live.pathname ?? "").trim();
+ if (!pattern) {
+ // strip leading slash only
+ return { data: url.pathname.replace(/^\/+/, "") };
+ }
+ // Anchor to start. If you intend a true regex, keep as-is; otherwise escape:
+ // const safe = pattern.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
+ const regex = new RegExp(`^(?:${pattern})`);
+ const slug = url.pathname.replace(regex, "").replace(/^\/+/, "");
+ return { data: slug };
+};🤖 Prompt for AI Agents
In website/functions/requestToPathname.ts around lines 16 to 26, the function
ignores the passed-in props and builds an unescaped, unanchored regex from
ctx.state.$live.pathname which can over-match; instead choose the basePath as
props?.pathname ?? ctx.state.$live.pathname, escape any regex metacharacters in
that value, anchor it to the start of the pathname (prepend ^) so only a leading
match is removed, and use a single replace (not a global replace) to strip the
basePath; ensure you coerce the chosen pathname to a string and handle
null/undefined safely.
| if (routePart.startsWith("}?")) { | ||
| if (routePart.includes("*")) { | ||
| return acc - 2; | ||
| } | ||
| return acc - 1; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Validate rank logic for optional/wildcard segments starting with }?.
Ensure your route syntax can actually produce segments that begin with "}?". If not, this branch never triggers or could mask other cases. Add a unit test with representative patterns to confirm ordering.
🏁 Script executed:
#!/bin/bash
# List route templates that contain '}' at segment start
rg -nP '(^|/)}\?' --type=ts -C2 website | sed -n '1,200p'Length of output: 0
I need to better understand the route template syntax and how segments are constructed. Let me examine the router.ts file and search for test cases or route examples.
🏁 Script executed:
#!/bin/bash
# Read the router.ts file to understand context
cat website/handlers/router.tsLength of output: 6575
Let me search for test files and route definition examples to understand what route patterns actually exist:
🏁 Script executed:
#!/bin/bash
# Search for test files that might contain route examples
fd -e test.ts -e test.tsx -e _test.ts -o -name '*test*.ts' website | head -20Length of output: 230
🏁 Script executed:
#!/bin/bash
# Look for route definitions and examples
rg -n "pathTemplate" --type=ts website | head -30Length of output: 2415
🏁 Script executed:
#!/bin/bash
# Search for what patterns actually contain } character
rg -nP '\}' --type=ts website/handlers/router.ts -C3Length of output: 6499
🏁 Script executed:
#!/bin/bash
# Look at flags/audience.ts to understand route structure
cat website/flags/audience.ts 2>/dev/null || echo "File not found"Length of output: 1879
Let me search more broadly for any route patterns that might explain the }? syntax, and check URLPattern usage:
🏁 Script executed:
#!/bin/bash
# Search for actual values assigned to pathTemplate
rg -nP 'pathTemplate\s*[=:]' --type=ts -A1 website | head -50Length of output: 2659
🏁 Script executed:
#!/bin/bash
# Look for any patterns with { or } characters that might be encoded or escaped
rg -nP '[{}<>]' --type=ts website/loaders/options/routes.ts -C2Length of output: 814
🏁 Script executed:
#!/bin/bash
# Search for imports or usage of URLPattern to understand the syntax
rg -n "URLPattern\|urlPatternCache" --type=ts website -C2 | head -60Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are any comments about route syntax or rankRoute logic
rg -n "rankRoute\|route.*syntax\|pattern.*format" --type=ts website -C3Length of output: 0
Let me search for any test files related to routing to confirm there's no test coverage:
🏁 Script executed:
#!/bin/bash
# Search for any test or spec files related to routing
find website -type f \( -name "*.test.ts" -o -name "*.spec.ts" -o -name "*_test.ts" \) | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for rankRoute function usage and calls
rg -n "rankRoute" --type=ts websiteLength of output: 199
🏁 Script executed:
#!/bin/bash
# Check if URLPattern is used and what patterns are created
grep -n "URLPattern\|new URLPattern" website/handlers/router.tsLength of output: 200
Remove dead code from rankRoute or add tests demonstrating when }? pattern can occur.
The }? check in the rankRoute function (line 29) appears unreachable: no route patterns in the codebase use this syntax, and it doesn't match standard URLPattern Web API syntax (which uses :param, *, **). Either remove this branch or add unit tests with concrete route examples that would trigger it.
🤖 Prompt for AI Agents
In website/handlers/router.ts around lines 29 to 34, the branch checking for
routePart.startsWith("}?") appears dead/unreachable; either remove this branch
to simplify rankRoute or add unit tests demonstrating a real route pattern using
"}?". Fix by choosing one: (A) delete the entire if (routePart.startsWith("}?"))
{ ... } block and adjust any dependent logic/tests accordingly, or (B) keep it
but add unit tests with concrete route strings that include the "}?"/wildcard
combinations (including cases with and without "*") that assert the decrement
behavior (acc -2 vs acc -1) so the branch is covered; ensure tests reference
existing route parsing expectations and update comments to explain why the
pattern is supported.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
website/components/OneDollarStats.tsx (2)
51-66: Fix: accidental invocation of subscribe return; add DECO guard; null‑safe page.idCalling the return value of
subscribe(})();) will throw at runtime. Also,window.DECOmay not exist yet, andpage.idis accessed without a null check.Apply:
- globalThis.window.DECO.events.subscribe((event) => { + if (globalThis.window.DECO?.events?.subscribe) { + globalThis.window.DECO.events.subscribe((event) => { if (!event || event.name !== "deco") { return; } - if (event.params) { - const { flags, page } = event.params; + if (event.params) { + const { flags, page } = event.params; if (Array.isArray(flags)) { for (const flag of flags) { props[flag.name] = truncate(flag.value.toString()); } } - props["pageId"] = truncate(`${page.id}`); + if (page?.id != null) { + props["pageId"] = truncate(String(page.id)); + } } trackPageview(); - })(); + }); + }
34-47: Prevent duplicate PageView emissions from pushState/popstate + "deco"Confirmed: website/components/OneDollarStats.tsx both monkey‑patches history.pushState / listens to popstate and also uses the DECO "deco" subscription (subscribe(...)()) to call trackPageview — this can double‑count pageviews on SPA navigations.
Minimal dedupe (skip repeated same href within a short window):
- const trackPageview = () => globalThis.window.stonks?.view?.(props); + let lastHref = ""; + let lastTs = 0; + const trackPageview = () => { + const href = location.href.replace(/#.*$/, ""); + const now = Date.now(); + if (href === lastHref && now - lastTs < 500) return; + lastHref = href; lastTs = now; + globalThis.window.stonks?.view?.(props); + };Or remove the pushState/popstate hooks and rely only on the DECO event if that event is guaranteed for all client-side navigations.
🧹 Nitpick comments (6)
website/components/OneDollarStats.tsx (6)
48-50: Truncation says “2000 bytes” but slices 990 charactersMismatch and not byte‑safe for multi‑byte chars.
Byte‑aware truncation:
- // 2000 bytes limit - const truncate = (str: string) => `${str}`.slice(0, 990); + // 2000 bytes limit (UTF‑8) + const truncate = (val: unknown, maxBytes = 2000) => { + const s = typeof val === "string" ? val : String(val ?? ""); + const enc = new TextEncoder(); + if (enc.encode(s).length <= maxBytes) return s; + let lo = 0, hi = s.length; + while (lo < hi) { + const mid = (lo + hi) >> 1; + (enc.encode(s.slice(0, mid)).length <= maxBytes) ? lo = mid + 1 : hi = mid; + } + return s.slice(0, Math.max(0, lo - 1)); + };
75-84: Use Object.entries to avoid inherited keys and TS ignore
for..incan traverse prototype properties; also avoids the TS ignore.- for (const key in params) { - // @ts-expect-error somehow typescript bugs - const value = params[key]; + for (const [key, value] of Object.entries(params)) { if (value !== null && value !== undefined) { - values[key] = truncate( - typeof value !== "object" ? value : JSON.stringify(value), - ); + values[key] = truncate( + typeof value !== "object" ? value : JSON.stringify(value), + ); } }
97-101: Preconnect/dns-prefetch should target origin, not path; encode staticScript URLUsing
/eventspath for preconnect/dns‑prefetch is suboptimal; also query param should be URL‑encoded.function Component({ collectorAddress, staticScriptUrl }: Props) { const collector = collectorAddress ?? DEFAULT_COLLECTOR_ADDRESS; const staticScript = staticScriptUrl ?? DEFAULT_ANALYTICS_SCRIPT_URL; + const collectorOrigin = (() => { try { return new URL(collector).origin; } catch { return collector; } })(); return ( <Head> - <link rel="dns-prefetch" href={collector} /> + <link rel="dns-prefetch" href={collectorOrigin} /> <link rel="preconnect" - href={collector} + href={collectorOrigin} crossOrigin="anonymous" /> <script id="tracker" data-autocollect="false" data-hash-routing="true" data-url={collector} type="module" - src={`/live/invoke/website/loaders/analyticsScript.ts?url=${staticScript}`} + src={`/live/invoke/website/loaders/analyticsScript.ts?url=${encodeURIComponent(staticScript)}`} />Also applies to: 109-109
16-28: Add Window.DECO typingYou declared
Window.stonksbut notWindow.DECO, causing TS friction.declare global { interface Window { stonks: { event: ( name: string, params: Record<string, string | boolean>, ) => void; view: ( params: Record<string, string | boolean>, ) => void; }; + DECO?: { + events: { + subscribe: (fn: (event: unknown) => unknown) => unknown; + }; + }; } }
35-35: Typo“dimentions” → “dimensions”.
- // Flags and additional dimentions + // Flags and additional dimensions
30-33: Exported trackerOriginal is unused hereIf it’s intentional for external import, keep it. If not, remove to reduce bundle size.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
google-drive/utils/query.ts(1 hunks)google-drive/utils/types.ts(3 hunks)hubspot/actions/conversations/archiveThread.ts(0 hunks)hubspot/actions/conversations/updateThread.ts(0 hunks)website/components/OneDollarStats.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- hubspot/actions/conversations/updateThread.ts
- hubspot/actions/conversations/archiveThread.ts
✅ Files skipped from review due to trivial changes (2)
- google-drive/utils/query.ts
- google-drive/utils/types.ts
🔇 Additional comments (1)
website/components/OneDollarStats.tsx (1)
37-47: Order-of-execution noteEnsure the module tracker script initializes
window.stonksbeforeoneDollarSnippetruns. You’re using two deferred scripts; order in DOM should preserve execution, but race is still possible on slow networks.If races occur, gate
trackPageview()calls untilwindow.stonksis truthy, or buffer events.Also applies to: 51-66
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
website/components/OneDollarStats.tsx (1)
51-65: Fix subscription invocation bug.Line 65 ends with
})();which immediately invokes the return value ofsubscribe(). Ifsubscribe()returns an unsubscribe function (common pattern), this would immediately unsubscribe the handler, preventing it from receiving any events. This is inconsistent with the second subscription at line 86, which correctly doesn't invoke the return value.Apply this diff to fix the issue:
- })(); + });
🧹 Nitpick comments (2)
website/components/OneDollarStats.tsx (2)
48-49: Clarify the truncation limit.The comment mentions a 2000 bytes limit, but the truncate function uses 990 characters. Consider documenting why this specific value was chosen (e.g., accounting for multiple fields, safety margin, etc.).
30-32: Remove unused exporttrackerOriginal.The
trackerOriginalconstant is exported but never used anywhere in the codebase. Remove this dead code to reduce bundle size.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/components/OneDollarStats.tsx(1 hunks)
🔇 Additional comments (1)
website/components/OneDollarStats.tsx (1)
67-86: Event forwarding logic looks correct.The subscription correctly filters and forwards non-"deco" events to
window.stonks.event, merging accumulated props with event params. The truncation and serialization logic appropriately handles different value types.Note: The
@ts-expect-erroron line 78 is acceptable given the dynamic nature of event parameters, though consider adding a more specific comment explaining why the type assertion is needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @website/components/Image.tsx:
- Around line 104-115: The optimizeMagento function builds query params
incorrectly when height is optional and when fit==='cover'; change it so you
only set the canvas param if height is present (e.g., check typeof height ===
"number") to avoid "400:undefined", and for fit avoid setting an empty
value—only add url.searchParams.set("fit", "bounds") when opts.fit !== "cover"
(omit the fit param entirely for cover). Update the body of optimizeMagento to
conditionally add canvas and fit params using those checks.
🧹 Nitpick comments (1)
website/components/Image.tsx (1)
126-129: Consider more robust URL pattern matching for Magento detection.The substring check
originalSrc.includes("media/catalog/product")at line 126 could produce false positives, matching any URL that contains this text in query parameters, fragments, or unrelated paths.For more precise detection, consider checking the pathname specifically:
♻️ Proposed refactor
- if (originalSrc.includes("media/catalog/product")) { + try { + const url = new URL(originalSrc); + if (url.pathname.includes("/media/catalog/product/")) { + return optimizeMagento(opts); + } + } catch { + // Invalid URL, fall through + } + + if (originalSrc.startsWith("https://cdn.vnda.")) { return optimizeMagento(opts); } - - if (originalSrc.startsWith("https://cdn.vnda.")) {This checks the pathname component only and handles potential URL parsing errors gracefully.
| const optimizeMagento = (opts: OptimizationOptions) => { | ||
| const { originalSrc, width, height } = opts; | ||
|
|
||
| const url = new URL(originalSrc); | ||
| url.searchParams.set("width", `${width}`); | ||
| url.searchParams.set("height", `${height}`); | ||
| url.searchParams.set("canvas", `${width}:${height}`); | ||
| url.searchParams.set("optimize", "low"); | ||
| url.searchParams.set("fit", opts.fit === "cover" ? "" : "bounds"); | ||
|
|
||
| return url.href; | ||
| }; |
There was a problem hiding this comment.
Handle optional height parameter to prevent malformed URLs.
Line 110 constructs the canvas parameter using ${width}:${height}, but height is optional in the OptimizationOptions interface. When height is undefined, this will produce a malformed parameter like "400:undefined".
Additionally, line 112 sets the fit parameter to an empty string when opts.fit === "cover". This creates a query parameter with an empty value (fit=), which may not be the intended behavior—consider omitting the parameter entirely or using a different value.
🔧 Proposed fix
const optimizeMagento = (opts: OptimizationOptions) => {
const { originalSrc, width, height } = opts;
const url = new URL(originalSrc);
url.searchParams.set("width", `${width}`);
- url.searchParams.set("height", `${height}`);
- url.searchParams.set("canvas", `${width}:${height}`);
+ if (height) {
+ url.searchParams.set("height", `${height}`);
+ url.searchParams.set("canvas", `${width}:${height}`);
+ }
url.searchParams.set("optimize", "low");
- url.searchParams.set("fit", opts.fit === "cover" ? "" : "bounds");
+ if (opts.fit !== "cover") {
+ url.searchParams.set("fit", "bounds");
+ }
return url.href;
};📝 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 optimizeMagento = (opts: OptimizationOptions) => { | |
| const { originalSrc, width, height } = opts; | |
| const url = new URL(originalSrc); | |
| url.searchParams.set("width", `${width}`); | |
| url.searchParams.set("height", `${height}`); | |
| url.searchParams.set("canvas", `${width}:${height}`); | |
| url.searchParams.set("optimize", "low"); | |
| url.searchParams.set("fit", opts.fit === "cover" ? "" : "bounds"); | |
| return url.href; | |
| }; | |
| const optimizeMagento = (opts: OptimizationOptions) => { | |
| const { originalSrc, width, height } = opts; | |
| const url = new URL(originalSrc); | |
| url.searchParams.set("width", `${width}`); | |
| if (height) { | |
| url.searchParams.set("height", `${height}`); | |
| url.searchParams.set("canvas", `${width}:${height}`); | |
| } | |
| url.searchParams.set("optimize", "low"); | |
| if (opts.fit !== "cover") { | |
| url.searchParams.set("fit", "bounds"); | |
| } | |
| return url.href; | |
| }; |
🤖 Prompt for AI Agents
In @website/components/Image.tsx around lines 104 - 115, The optimizeMagento
function builds query params incorrectly when height is optional and when
fit==='cover'; change it so you only set the canvas param if height is present
(e.g., check typeof height === "number") to avoid "400:undefined", and for fit
avoid setting an empty value—only add url.searchParams.set("fit", "bounds") when
opts.fit !== "cover" (omit the fit param entirely for cover). Update the body of
optimizeMagento to conditionally add canvas and fit params using those checks.
What is this Contribution About?
Please provide a brief description of the changes or enhancements you are proposing in this pull request.
Issue Link
Please link to the relevant issue that this pull request addresses:
Loom Video
Demonstration Link
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.