-
Notifications
You must be signed in to change notification settings - Fork 3
Refactor #37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Refactor #37
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor by introducing an XMTP agent backend that extracts mentions and replies with a mini app URL, replacing authenticated frontend flows by removing
/api/auth/logout, disabling middleware auth checks, and switching page rendering to a member resolution view@game, extracts mentions, and replies with a URL containing?tags=; in DMs it responds to/startwith the URL, and logs onstartin index.ts, including a newextractMentionsutility./api/auth/logoutroute in route.ts and returningNextResponse.next()for all requests in middleware.ts.MemberRendererin MemberRenderer.tsx and updating the page in Page.tsx.📍Where to Start
Start with the XMTP agent entry point and message handlers in
index.tsin index.ts, then review the member resolution flow inMemberRendererin MemberRenderer.tsx.📊 Macroscope summarized ada29c3. 14 files reviewed, 34 issues evaluated, 33 issues filtered, 0 comments posted
🗂️ Filtered Issues
backend/index.ts — 0 comments posted, 5 evaluated, 5 filtered
process.env.XMTP_ENVis cast to a narrow union type withas "local" | "dev" | "production"and passed directly toAgent.createFromEnvwithout any runtime validation. At runtimeprocess.env.XMTP_ENVcan beundefinedor any arbitrary string (e.g.,"development","staging"). Because TypeScript erases the assertion, an invalid or undefined value will be passed through and may causeAgent.createFromEnvto throw or misconfigure the agent during startup. There is no guard, default, or explicit error handling, so the process may fail during initialization with a non-actionable error. Add a runtime check to validate and map env values (or provide a safe default) before callingcreateFromEnv, and surface a clear error if the value is invalid. [ Low confidence ]contentis used without a null/undefined guard. Ifctx.message.contentisnullorundefined, subsequent calls tocontent.includes(...)(group and DM checks) will throw aTypeError. There are no visible guards ensuringctx.message.contentis always a string. Add a nullish check and default to an empty string or bail out safely before calling.includes()or passing toextractMentions. [ Low confidence ]const tagsString =?tags=${mentions.join(",")}. If any mention contains characters that should be percent-encoded in URLs (e.g., Unicode ellipsis…from shortened addresses), the generated link may be malformed or parsed inconsistently by clients or the frontend. UseencodeURIComponentfor each tag orURLSearchParams` to build the query string safely. [ Already posted ]@-prefixed full address, and "Remove @"; however, the regex/(0x[a-fA-F0-9]{40})\b/gmatches full addresses without requiring@, and the code pushes matches as-is without removing@. This inconsistency creates uncertainty about intended behavior (should standalone0x...be matched or only@0x...mentions?), which can lead to unexpected extra matches. [ Low confidence ]\b(?<!@)([\w-]+(?:\.[\w-]+)*\.eth)\buses a negative lookbehind(?<!@). JavaScript lookbehind is not supported in older Node.js runtimes; if the backend runs on such a runtime, this will cause a syntax error at load/parse time, crashing the service. Consider replacing with an alternative (e.g., using a capturing group with a negative lookbehind emulation) or ensure the runtime supports lookbehind. [ Low confidence ]frontend/middleware.ts — 0 comments posted, 1 evaluated, 1 filtered
NextResponse.next()for all requests, removing prior authentication checks and the injection of thex-user-idheader. This changes the externally visible contract: requests that previously returned401for missing/invalid tokens and those that hadx-user-idpropagated will now pass through unauthenticated and without user context. Downstream handlers that rely on these invariants may misbehave or expose protected endpoints without guardrails. [ Low confidence ]frontend/scripts/dev-with-ngrok.ts — 0 comments posted, 7 evaluated, 7 filtered
checkEnvFileandloadEnvFilecan cause silent truncation.checkEnvFileflags inline comments only when there is a space before#(conditionvalue.includes(' #'), line 49), butloadEnvFilestrips any#in an unquoted value (lines 87-90). For example,PASSWORD=abc#123would be truncated toabcbyloadEnvFilewithout being flagged bycheckEnvFile, resulting in silent data loss. [ Already posted ]loadEnvFilefails to handle inline comments following a quoted value, e.g.,KEY="foo" # cmtorKEY='foo' # cmt. Because it skips comment stripping for any value that starts with a quote (lines 87-90) and only removes quotes when both start and end quotes are present at the string ends (lines 93-96), a value like"foo" # cmtdoes not end with a quote and will be set verbatim (including the trailing comment and quotes) intoprocess.env. This produces incorrect environment variable values. [ Already posted ]loadEnvFileuses a truthiness checkif (key && !process.env[key])before settingprocess.env[key] = value(lines 99-101). This treats an existing empty string value as “unset,” causing it to be overwritten. In Node.js, environment variables are strings; an empty string is a valid set value. The check should use existence (e.g.,Object.prototype.hasOwnProperty.call(process.env, key)) rather than truthiness to avoid overwriting empty values. [ Already posted ]nextServeris declared asChildProcessbut is not initialized at declaration (let nextServer: ChildProcess;). At runtime this meansnextServerisundefineduntil assigned. Any code that usesnextServerprior to assignment (e.g., callingnextServer.kill()inside cleanup) will throw aTypeError(Cannot read properties of undefined). This introduces a crash path whenevercleanup()is invoked beforenextServeris set or if its initialization fails. [ Low confidence ]const output = data.toString();then matching within that chunk), but streams may split lines across multipledataevents. If the "Local:" line is split across chunks, the regex will not match, causingdetectedPortto remain unset andstartNgrok()to never be called. Buffering until newline boundaries or accumulating until a match is found is needed for correctness. [ Low confidence ]output.match(/Local:\s+http:\/\/localhost:(\d+)/)is brittle and may fail to detect the port in common situations: ANSI color codes in output, different host strings (e.g.,127.0.0.1), HTTPS (https://), different capitalization/formatting by Next.js, or locale variations. Failure to match leavesdetectedPortunset and preventsstartNgrok()from being invoked, breaking the dev flow. [ Low confidence ]cleanupfunction callsnextServer.kill()without checking whethernextServeris defined or still running, and then immediately callsprocess.exit(0). IfnextServerisundefined,cleanupwill throw before exiting, leaving the process in an inconsistent state (no exit, no cleanup). IfnextServeris defined but already exited or the kill fails (returnsfalse), the process will still exit immediately, potentially leaving an orphaned or mismanaged child state without confirming termination (no'exit'/'close'event handling). There is no single paired cleanup or ordering guarantee to ensure the child terminates before the parent exits, nor guards to makecleanupidempotent or safe under multiple invocations. [ Low confidence ]frontend/src/app/page.tsx — 0 comments posted, 1 evaluated, 1 filtered
generateMetadatano longer usesconversationIdto construct a per-conversation OG image URL (previously${env.NEXT_PUBLIC_URL}/api/og/image/${conversationId}) and instead always uses the static default image (${env.NEXT_PUBLIC_URL}/frame-default-image.png). It also removes Twittercreator,siteId, andcreatorIdfields. If downstream clients rely on dynamic, conversation-specific OG images or those Twitter fields, this change alters externally visible behavior. If intentional, it should be explicitly documented; otherwise, restore dynamic OG generation whenconversationIdis present and keep the removed fields if still required by consumers. [ Low confidence ]frontend/src/components/MemberRenderer.tsx — 0 comments posted, 8 evaluated, 8 filtered
isResolvingis set to true and only set to false afterPromise.allresolves. If any individualresolveIdentifierhangs (e.g., network stall), the entire batch never completes and the UI remains stuck showing “Resolving...” with no timeout or per-item fallback. Consider per-item timeouts or settling withPromise.allSettled, and drivingisResolvingfrom per-item state or a finite timeout. [ Low confidence ]resolveTagsawaitsPromise.alland sets state afterward without checking if the component is still mounted. If the component unmounts during resolution, React can warn and work may leak; safer to include an effect cleanup that sets adidCancelflag and check it beforesetMembers/setIsResolving. [ Already posted ]resolveTagsinsideuseEffectdoes not include any cancellation or sequencing guard. IfsearchParamsordefaultTagschanges while a previous resolution is in-flight, the olderPromise.allcan resolve later and callsetMembers(resolvedMembers)andsetIsResolving(false), overwriting newer state with stale results and finalizing as not resolving. Use an abort/sequence token and check it before applying results, with a cleanup function that flips the token on effect re-run/unmount. [ Low confidence ]tagsare passed through as-is, allowing duplicate entries. This causes duplicate work, duplicate list items, and key collisions. Dedupe the array (e.g., with aSet) before resolving and rendering. [ Already posted ]defaultTagsis unstable: the default parameterdefaultTags = []creates a new array each render when the prop is omitted, causinguseEffectto re-run on every render. This leads to repeated work and can trigger multiple concurrent resolutions if state changes. Use a stable dependency (e.g., derive a string key, or memoizedefaultTagsat the call site) or exclude it and explicitly watch a stable representation. [ Low confidence ]useEffectruns,membersis empty andisResolvingis false, so the component briefly renders the empty-state UI even when tags are present. This causes a visible flicker before showing resolving state. InitializeisResolvingbased on presence of tags (e.g., compute tags synchronously fromsearchParamsand set initial state accordingly) or gate the empty-state on an explicit “initialized” flag. [ Low confidence ]key={member.identifier}. If thetagsarray contains duplicates (allowed by current parsing and no de-duplication), keys will collide causing React warnings and unstable UI reconciliation. Either enforce uniqueness when parsing or use a composite key (e.g.,identifier + '-' + index) after de-duplication. [ Already posted ]navigator.clipboard.writeText(member.address!)is called withoutawait/catch. If permissions are denied or the API is unavailable, this can raise an unhandled promise rejection and provides no user feedback. Wrap in try/catch or.catchand optionally provide UI feedback. [ Low confidence ]frontend/src/lib/frame.ts — 0 comments posted, 5 evaluated, 5 filtered
frame.iconUrl,frame.homeUrl,frame.splashImageUrl, andframe.webhookUrl, but the code relies onenv.NEXT_PUBLIC_URLwhich is validated as a generic URL (z.string().url()), allowinghttp://schemes. IfNEXT_PUBLIC_URLis set to anhttporigin (a reachable input under the current guards), the function will emit a manifest withhttpURLs, violating the external contract and likely being rejected or causing runtime failures when Farcaster fetches resources. A concrete fix is to enforcehttpsin validation (e.g., custom refinement) or to assert and throw before emitting the manifest whenenv.NEXT_PUBLIC_URLisn't HTTPS. [ Low confidence ]env.NEXT_PUBLIC_URLusing naive string concatenation forframe.iconUrl,frame.splashImageUrl, andframe.webhookUrl. IfNEXT_PUBLIC_URLends with a trailing slash, the result contains double slashes (e.g.,https://example.com//icon.png). IfNEXT_PUBLIC_URLincludes a path (e.g.,https://example.com/app), concatenation yieldshttps://example.com/app/icon.pngrather than anchoring at the origin, which may not be intended and can produce invalid or unexpected endpoints for Farcaster. Use URL-safe joining (e.g.,new URL('/icon.png', env.NEXT_PUBLIC_URL).toString()) to normalize slashes and anchor paths correctly. [ Low confidence ]frame.iconUrlmax 1024,frame.homeUrlmax 1024,frame.splashImageUrlmax 32,frame.webhookUrlmax 1024, per the inline comments and referenced schema). The code does not enforce or validate these upper bounds. Given the current env validation (z.string().url().min(1)), excessively longNEXT_PUBLIC_URLvalues are reachable, which can yield derived URLs exceeding these limits, causing manifest validation failures at runtime. Add explicit length checks/refinements onNEXT_PUBLIC_URLor validate the resulting constructed URLs before returning the manifest. [ Low confidence ]getFrameMetadataconstructs a query string by naively doingObject.entries(_params).map(([key, value]) =>${key}=${value}). This has multiple runtime issues: (1) values are not URL-encoded, so reserved characters will produce malformed URLs; (2)undefinedvalues become the string "undefined" (e.g.,key=undefined), which is incorrect; (3) array values (string[]) stringify to comma-joined values (e.g.,a=1,2), which is not a valid or predictable encoding for multiple values; and (4) keys with empty arrays or empty strings are treated as present. The resultingurlcan be malformed or misinterpreted by consumers. UseURLSearchParamsand filter outundefinedvalues, and explicitly handle array values. [ Already posted ]getFrameMetadatatreatsconversationIdas a generic truthy check to choose the button title (Open Conversation in XMTPvsLaunch XMTP MiniApp). Because_params.conversationIdcan bestring | string[] | undefined, this yields incorrect titles for some reachable inputs: an empty array[]or an array containing an empty string['']are both truthy, while an empty string''is falsy. The result is inconsistent UI text that does not actually reflect the presence of a usable conversation id. Normalize the param to a non-empty string (e.g., handlestring[]by choosing the first defined non-empty string) before deciding. [ Low confidence ]frontend/src/lib/resolver.ts — 0 comments posted, 3 evaluated, 3 filtered
matchShortenedAddress(/^(0x[a-fA-F0-9]+)(?:…|\. {2,3})([a-fA-F0-9]+)$/)permits arbitrarily short prefix/suffix segments (e.g.,0x…1), greatly increasing the risk of false positives when matching against a list of addresses. Without a minimum length constraint on the prefix and suffix, the function may match many candidates, leading to incorrect selection (compounded by the "first match" behavior). Enforce minimum lengths (e.g., at least 2–4 nibbles on each side) or validate that the combined prefix+suffix length equals the expected 40 hex chars to reduce ambiguity. [ Low confidence ]matchShortenedAddressreturns the firstfullAddressthat matches the prefix/suffix check without verifying uniqueness. If multiple entries infullAddressessatisfy the same shortened pattern, the function silently returns whichever appears first, which can produce incorrect or non-deterministic results depending on ordering. For correctness, the function should detect multiple matches and either return null with an error or enforce a deterministic, documented disambiguation rule. [ Low confidence ]resolveIdentifieruses a non-anchored regex/0x[a-fA-F0-9]+(?:…|\. {2,3})[a-fA-F0-9]+/which can match a substring anywhere in the input. This can misclassify an identifier that merely contains a shortened-address-like substring (e.g., "foo 0xabc…def bar") as a shortened address. The function then callsmatchShortenedAddress(which expects the whole string to be a shortened address due to its^...$anchors), getsnull, and returnsnullearly instead of attempting the domain resolution path. This yields an incorrect null result when the rest of the identifier could have been resolvable (e.g., a domain). The regex should be anchored (e.g.,^...$) to only treat the whole identifier as a shortened address. [ Already posted ]frontend/src/providers/index.tsx — 0 comments posted, 1 evaluated, 1 filtered
Providerscomponent no longer wraps itschildrenwithMiniAppWalletProviderandXMTPProvider, and thecookiesprop was removed. This breaks the externally visible contract ofProvidersfor any child component expecting wallet or XMTP contexts to be present (e.g., hooks that require those providers). At runtime, such components will throw (typical patterns:useMiniAppWallet must be used within MiniAppWalletProvideror similar) or silently misbehave because required initialization (e.g., wallet/session hydration usingcookies, XMTP client setup) no longer occurs. In the direct call chain,RootLayoutwas updated to stop passingcookies, but all children now run without those providers, which is a contract parity violation and can cause runtime failures or degraded behavior throughout the app. [ Low confidence ]frontend/tailwind.config.ts — 0 comments posted, 2 evaluated, 2 filtered
./src/examples/**/*.{js,ts,jsx,tsx,mdx}glob from the Tailwindcontentarray. If any components/pages withinsrc/examplesare actually part of the running app (e.g., imported into other components, or routed), Tailwind will no longer scan them for class names. This will cause their Tailwind classes to be purged and result in missing styles at runtime. Ifsrc/examplesis strictly non-runtime/demo content that is never imported or routed, this is fine; otherwise, it’s a breaking change that can silently drop styles. [ Low confidence ]tailwindcss-inner-borderplugin from thepluginsarray. If any components rely on utilities provided by this plugin (e.g., inner border utilities), those classes will no longer be generated, resulting in missing styles at runtime. This is a contract change: previously available utilities are now absent. If there are no usages, this is harmless; otherwise, it will produce silent UI regressions. [ Low confidence ]