Tavano/fix vtex and figma #1374
Conversation
WalkthroughIntroduces nullable Figma client in state and adds runtime guards across Figma loader functions to throw "Figma client not found" when absent. Slack files receive signature and formatting adjustments without logic changes. VTEX module logs props and makes publicUrl check optional-safe. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant App as App (figma/mod.ts)
participant State as State
participant Loader as Figma Loader
participant Figma as Figma API
Caller->>App: init(props)
App->>App: if props.accessToken\n create FigmaClient\n else null
App-->>State: { figma: FigmaClient | null }
Caller->>Loader: call loader(ctx)
Loader->>Loader: guard ctx.figma?\n if not, throw "Figma client not found"
alt figma present
Loader->>Figma: request (getFile/getImages/...) with params
Figma-->>Loader: response
Loader-->>Caller: transformed result
else figma missing
Loader-->>Caller: error thrown
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Comment |
Tagging OptionsShould a new tag be published when this PR is merged?
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
figma/loaders/getImagesToFills.ts (1)
29-38: Possible undefined return when images are missing
imagesmay beundefined; the else branch assignsimagesToReturn = images, causing the function to returnundefinedwhile the return type isRecord<string, string>.- const images = (await ctx.figma.getImageFills(fileKey))?.meta?.images; + const images = (await ctx.figma.getImageFills(fileKey))?.meta?.images ?? {}; @@ - } else { - imagesToReturn = images; - } + } else { + imagesToReturn = images; + }
🧹 Nitpick comments (9)
figma/loaders/getNodes.ts (1)
60-62: Prefer returning a typed error over throwing a generic ErrorThrowing a plain Error likely surfaces as a 500. Consider returning a FigmaResponse with
errto match downstream handling patterns.- if (!ctx.figma) { - throw new Error("Figma client not found"); - } + if (!ctx.figma) { + return { err: "Figma client not found" } as FigmaResponse<FileNodesResponse>; + }figma/loaders/getFileImages.ts (1)
93-96: Align error handling with FigmaResponse contractReturn a typed
{ err }instead of throwing to avoid 500s and keep callers’response.errchecks working.- if (!ctx.figma) { - throw new Error("Figma client not found"); - } + if (!ctx.figma) { + return { err: "Figma client not found" } as FigmaResponse<{ images: Record<string, string | null> }>; + }figma/loaders/getImagesSpecificNode.ts (1)
32-34: Consistent typed error instead of generic throwFor consistency with other loaders and consumers that check
response.err, prefer returning a typed error.- if (!ctx.figma) { - throw new Error("Figma client not found"); - } + if (!ctx.figma) { + return { err: "Figma client not found" } as FigmaResponse<{ images: Record<string, string> }>; + }figma/loaders/getImagesToFills.ts (1)
26-28: Consider a typed/HTTP error instead of generic throwOptional: return an HTTP 401/400 or a typed object for consistency across loaders.
- if (!ctx.figma) { - throw new Error("Figma client not found"); - } + if (!ctx.figma) { + throw new Error("Figma client not found"); + }vtex/mod.ts (1)
102-102: Remove noisy console logAvoid logging props in production code.
- console.log("props", props);figma/loaders/getComponents.ts (1)
38-40: Prefer typed error return for consistencyReturning
{ err }keeps behavior consistent with subsequentresponse.errchecks.- if (!ctx.figma) { - throw new Error("Figma client not found"); - } + if (!ctx.figma) { + return { err: "Figma client not found" } as FigmaResponse<FigmaFile>; + }slack/actions/files/download.ts (2)
40-42: Include status code in error for easier triage.
Minor ergonomics improvement.- message: `Failed to download file: ${ - fileResponse.statusText || "Unknown error" - }`, + message: `Failed to download file: ${fileResponse.status} ${fileResponse.statusText || "Unknown error"}`,
56-56: Avoid O(n) string concat for base64; use std encoder (faster, lower memory).
The current Array(...).map(...).join("") pattern is costly for large files.- // Convert to base64 - const base64 = btoa( - Array(bytes.length) - .fill("") - .map((_, i) => String.fromCharCode(bytes[i])) - .join(""), - ); + // Convert to base64 + const base64 = encodeBase64(bytes);Add import at top:
import { encodeBase64 } from "jsr:@std/encoding/base64";If you prefer pinned std, replace the import path accordingly.
slack/client.ts (1)
436-439: Consider adding timeouts/retries to all Slack fetches.
External calls currently have no timeouts or backoff. Recommend a small helper to add AbortController timeouts and optional retry-on-429/5xx.Example helper:
async function fetchJson(url: string, init: RequestInit = {}, timeoutMs = 10000) { const ac = new AbortController(); const t = setTimeout(() => ac.abort(), timeoutMs); try { const res = await fetch(url, { ...init, signal: ac.signal }); return res.json(); } finally { clearTimeout(t); } }Then swap return fetch(...).json() with fetchJson(...).
Also applies to: 480-483
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
figma/loaders/getComponents.ts(1 hunks)figma/loaders/getFileImages.ts(1 hunks)figma/loaders/getImagesSpecificNode.ts(1 hunks)figma/loaders/getImagesToFills.ts(1 hunks)figma/loaders/getNodes.ts(1 hunks)figma/loaders/getSimplified.ts(1 hunks)figma/loaders/getSimplifiedNodes.ts(1 hunks)figma/mod.ts(2 hunks)slack/actions/dms/send.ts(1 hunks)slack/actions/files/download.ts(2 hunks)slack/actions/files/info.ts(1 hunks)slack/client.ts(4 hunks)vtex/mod.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
figma/mod.ts (1)
figma/client.ts (1)
FigmaClient(84-294)
🔇 Additional comments (10)
slack/actions/files/info.ts (1)
25-27: LGTM — formatting-onlyTemplate literal reformatting doesn’t alter runtime output.
figma/mod.ts (1)
14-15: State update looks goodMaking
figmanullable matches the new guards elsewhere.slack/actions/dms/send.ts (2)
28-30: Signature formatting only — OK.
No behavioral change.
38-40: Error template reformat — OK.
Message content unchanged.slack/actions/files/download.ts (2)
17-19: Signature formatting only — OK.
No runtime impact.
46-47: Content-Type defaulting — OK.
Sane default retained.slack/client.ts (2)
436-439: Signature reformat — OK.
No logic change.
480-483: Signature reformat — OK.
No logic change.figma/loaders/getSimplified.ts (1)
63-65: Null-guard added — OK. Verify error contract.
Throwing will bubble a 500 unless caught upstream. If callers expect a FigmaResponse error shape, prefer returning a structured error instead of throwing.Would you like a patch returning a typed FigmaResponse error to align with other loaders’ contracts?
figma/loaders/getSimplifiedNodes.ts (1)
63-67: Null-guard + local alias — OK. Verify error contract.
Same note as getSimplified.ts regarding throw vs structured error.Want a follow-up patch to return a FigmaResponse error instead of throwing here?
| const figma = props.accessToken | ||
| ? new FigmaClient( | ||
| typeof props.accessToken === "string" | ||
| ? props.accessToken | ||
| : props.accessToken.get()!, | ||
| ) | ||
| : null; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid non-null assertion and handle Secret.get() safely
props.accessToken being truthy doesn’t guarantee Secret.get() returns a string. This can create a client with an invalid token.
- const figma = props.accessToken
- ? new FigmaClient(
- typeof props.accessToken === "string"
- ? props.accessToken
- : props.accessToken.get()!,
- )
- : null;
+ const token = typeof props.accessToken === "string"
+ ? props.accessToken.trim()
+ : props.accessToken.get()?.trim();
+ const figma = token ? new FigmaClient(token) : null;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In figma/mod.ts around lines 26–33, avoid using the non-null assertion on
props.accessToken.get(); instead obtain the token into a local variable (handle
both string and Secret cases), check that the result is a defined, non-empty
string, and only then pass it to new FigmaClient; if the token is
missing/invalid, set figma to null (or throw a clear error) so you don’t
construct a client with an invalid token. Ensure you do not use "!" and handle
the Secret.get() return safely.
| return { | ||
| success: false, | ||
| message: "Only Slack-hosted HTTPS file URLs are allowed.", | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden against cross-host redirects to prevent token leakage.
Even with an allowlist, automatic redirects could send Authorization headers to non-Slack hosts. Fetch with redirect: "manual" in the Slack client and validate Location/response.url stays allowed before following.
Apply in slack/client.ts:
- downloadFile(fileUrl: string): Promise<Response> {
- return fetch(fileUrl, { headers: this.botHeaders });
- }
+ downloadFile(fileUrl: string): Promise<Response> {
+ return fetch(fileUrl, { headers: this.botHeaders, redirect: "manual" });
+ }Then here, follow only safe redirects:
- const fileResponse = await ctx.slack.downloadFile(props.fileUrl);
+ let fileResponse = await ctx.slack.downloadFile(props.fileUrl);
+ // Follow at most one redirect, staying within the allowlist
+ if (fileResponse.status >= 300 && fileResponse.status < 400) {
+ const location = fileResponse.headers.get("location");
+ if (!location) {
+ return { success: false, message: "Redirect without Location header." };
+ }
+ const next = new URL(location, props.fileUrl);
+ if (next.protocol !== "https:" || !allowed(next.hostname.toLowerCase())) {
+ return { success: false, message: "Redirect to non-Slack host blocked." };
+ }
+ fileResponse = await ctx.slack.downloadFile(next.toString());
+ }Committable suggestion skipped: line range outside the PR's diff.
| console.log("props", props); | ||
| const publicUrl = _publicUrl?.startsWith("https://") | ||
| ? _publicUrl | ||
| : `https://${_publicUrl}`; |
There was a problem hiding this comment.
Bug: may produce "https://undefined" when publicUrl is absent
The optional chain makes the condition false when _publicUrl is undefined, yielding https://${_publicUrl} => https://undefined.
- console.log("props", props);
- const publicUrl = _publicUrl?.startsWith("https://")
- ? _publicUrl
- : `https://${_publicUrl}`;
+ if (!_publicUrl?.trim()) {
+ throw new Error("VTEX publicUrl is required");
+ }
+ const publicUrl = _publicUrl.startsWith("https://")
+ ? _publicUrl
+ : `https://${_publicUrl}`;📝 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.
| console.log("props", props); | |
| const publicUrl = _publicUrl?.startsWith("https://") | |
| ? _publicUrl | |
| : `https://${_publicUrl}`; | |
| if (!_publicUrl?.trim()) { | |
| throw new Error("VTEX publicUrl is required"); | |
| } | |
| const publicUrl = _publicUrl.startsWith("https://") | |
| ? _publicUrl | |
| : `https://${_publicUrl}`; |
🤖 Prompt for AI Agents
In vtex/mod.ts around lines 102 to 105, the current logic can produce
"https://undefined" because it uses optional chaining only in the startsWith
check; instead, first check that _publicUrl is defined/truthy before calling
startsWith or interpolating it. Update the assignment so it: 1) returns
_publicUrl unchanged if it's truthy and already starts with "https://", 2)
prefixes "https://" only when _publicUrl is a non-empty string that does not
already start with "https://", and 3) leaves publicUrl undefined (or another
safe default) when _publicUrl is missing. Ensure you perform the null/undefined
check before string operations to avoid "https://undefined".
It was broken app runtime, then we were indexing MCP of vtex and figma with 0 tools.
Summary by CodeRabbit
Bug Fixes
Chores