Skip to content
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

[FH-2] Open Frames Implementation (#163) #185

Closed
wants to merge 1 commit into from

Conversation

developerfred
Copy link

@developerfred developerfred commented Oct 4, 2024

#163

  • enable support lens frames
  • enable support XMTP
  • add validators
  • add news endpoints

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a loading component with humorous messages during loading.
    • Added functionality for handling Lens frame requests and their validation.
    • Enhanced error handling for dynamic page rendering.
  • Improvements

    • Updated payload validation process for frame requests.
    • Improved metadata handling for frames, allowing for more detailed information.
  • Bug Fixes

    • Refined error handling in various functions to ensure consistent logging and responses.
  • Documentation

    • Updated TypeScript definitions to improve clarity and structure for frame-related metadata.
  • Chores

    • Updated package dependencies and scripts for better development experience.

- enable support lens frames
- enable support XMTP
- add validators
- add news endpoints
Copy link
Contributor

coderabbitai bot commented Oct 4, 2024

Walkthrough

The pull request introduces a series of changes across multiple files in the application, primarily focusing on enhancing payload validation, error handling, and overall code organization. Key modifications include the introduction of new types and interfaces, the addition of a loading component, and updates to existing functions to improve their structure and readability. Notably, several new functions for validating frame requests and handling metadata have been added, while existing functions have been refactored for better modularity and maintainability. Formatting adjustments and new dependencies have also been introduced.

Changes

File Path Change Summary
app/(app)/f/[frameId]/[handler]/[[...query]]/route.ts Reorganized imports, added validateFramePayload, modified POST function for payload validation, updated error handling, standardized code formatting, and added a new processFrame method.
app/(app)/f/[frameId]/interactions/route.ts Minor formatting change with a newline added at the end of the file; no functional changes.
app/(app)/f/[frameId]/show/page.tsx Added conditional check for the existence of the Page component to improve error handling.
app/(app)/loading.tsx Introduced a loading component with humorous messages and a progress bar.
app/(app)/of/[frameId]/[handler]/[[query]]/route.ts New file for handling POST requests related to frames, including frame retrieval, payload validation, and error handling. Added POST and processFrame functions.
app/(app)/p/[frameId]/[handler]/[[...query]]/route.ts Updated imports, modified POST function for improved error handling and payload validation.
app/(app)/page.tsx Cosmetic changes in variable declarations for consistency; no functional changes.
app/(app)/templates/page.tsx Formatting adjustments for improved readability; no functional changes.
app/layout.tsx Introduced a layout component with SessionProvider integration and comprehensive metadata.
app/manifest.ts Minor formatting change with a newline added at the end of the file; no functional changes.
app/robots.ts Minor modification with a newline added at the end of the file; no functional changes.
lib/farcaster.d.ts Added OpenFrameMetadata and OpenFrameButton interfaces, modified FrameRequest, and updated BuildFrameData.
lib/frame.ts Refactored functions to centralize authentication handling and improve query construction; modified several function signatures.
lib/lens-frames.ts New file defining LensFrameRequest and LensFrameValidationResult interfaces, and a function for validating lens frame requests.
lib/openframes.ts New file for managing open frame metadata, defining ClientProtocol, OpenFrameMetadata, and OpenFrameButton, and implementing a function to construct metadata.
lib/serve.ts Enhanced metadata handling in buildFrame functions, updated function signatures to include optional metadata parameters.
lib/storage.ts Minor change with a newline at the end; no functional changes.
lib/template.ts Introduced Template type, modified functions to return Promise<Template[]>, and updated filtering logic.
lib/types.d.ts Added OpenFrameRequest interface and new types for frame and template handlers.
lib/validation.ts Added a new asynchronous function validateFramePayload for structured payload validation based on client protocols.
package.json Updated scripts for linting and formatting, added new dependencies for @lens-protocol/client and @xmtp/frames-validator.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 32

🧹 Outside diff range and nitpick comments (10)
app/(app)/f/[frameId]/show/page.tsx (1)

Line range hint 1-45: Approved: Improved error handling for dynamic imports

The addition of the conditional check for the Page component enhances the robustness of the code by ensuring that a valid page template is available before rendering. This change aligns well with the PR objectives of implementing lens frames support.

For consistency, consider applying the same error handling pattern for the frame fetch:

const frame = await client
    .select()
    .from(frameTable)
    .where(eq(frameTable.id, params.frameId))
    .get()

if (!frame) {
    return notFound()
}

This ensures that the notFound() function is consistently used as a return statement, matching the usage in the new conditional check.

package.json (3)

9-11: Approve linting and formatting changes with a suggestion.

The switch to Biome for linting, formatting, and checking is a good improvement. It provides a more comprehensive toolset for code quality.

Consider combining the lint, format, and check scripts into a single validate script for convenience:

"validate": "biome lint . && biome format . && biome check --apply ."

This would allow running all checks with a single command.


28-28: Approve new dependencies with a suggestion.

The addition of @lens-protocol/client and @xmtp/frames-validator aligns well with the PR objectives of enabling support for lens frames, XMTP, and adding validators.

Consider adding these dependencies to your project's documentation or README file to inform other developers about the new functionalities and dependencies.

Also applies to: 61-61


103-103: Approve new dev dependency with a suggestion.

The addition of @biomejs/biome as a dev dependency is consistent with the changes in the scripts section. This ensures that the Biome tool is available for the new linting, formatting, and checking scripts.

Consider using caret versioning (^1.7.3) for consistency with other dependencies and to allow for compatible updates:

"@biomejs/biome": "^1.7.3"

This would allow for minor version updates that include bug fixes and new features while maintaining backwards compatibility.

lib/openframes.ts (1)

37-37: Avoid unnecessary type assertions with 'as'

On line 37, the type assertion as 'post' | 'post_redirect' | 'link' | 'mint' | 'tx' may be unnecessary if button.action is already correctly typed in BuildFrameData. Consider ensuring that button.action has the appropriate type to eliminate the need for this assertion.

app/(app)/p/[frameId]/[handler]/[[...query]]/route.ts (2)

60-66: Enhance error messaging for payload validation failures

The error response returns a generic message when validation fails. Providing more specific error information can aid in debugging and improve the developer experience.

Optionally, modify the error response to include details from the validation error:

return Response.json(
    { message: error instanceof Error ? error.message : 'Invalid frame payload' },
    { status: 400 }
);

Ensure that any sensitive information is not exposed in error messages.


87-88: Consider a robust logging mechanism for unexpected errors

Using console.error(error); may not be sufficient for error tracking in a production environment. Implementing a logging service or error tracking tool can help monitor and debug unexpected issues more effectively.

Consider integrating a logging library or service to handle error reporting:

- console.error(error);
+ logger.error('Unexpected error in handlerFn', error);

Ensure that the logger is properly configured to capture and store logs securely.

app/(app)/f/[frameId]/[handler]/[[...query]]/route.ts (1)

Line range hint 128-150: Optimize webhook dispatch with asynchronous handling

Currently, webhooks are being dispatched inside a loop without waiting for their completion. To improve efficiency and error handling, consider using Promise.all to run these fetch requests concurrently.

Apply this diff to modify the webhook dispatch:

-    for (const webhook of parameters?.webhooks || []) {
-        if (!webhookUrls?.[webhook.event]) {
-            continue;
-        }
-
-        fetch(webhookUrls[webhook.event], {
-            method: 'POST',
-            headers: { 'Content-Type': 'application/json' },
-            body: JSON.stringify({
-                event: webhook.event,
-                data: {
-                    ...webhook.data,
-                    createdAt: new Date().toISOString(),
-                },
-            }),
-        })
-            .then(() => {
-                console.log('Sent webhook');
-            })
-            .catch((e) => {
-                console.error('Error sending webhook', e);
-            });
-    }
+    await Promise.all(
+        (parameters?.webhooks || []).map(async (webhook) => {
+            if (!webhookUrls?.[webhook.event]) {
+                return;
+            }
+            try {
+                await fetch(webhookUrls[webhook.event], {
+                    method: 'POST',
+                    headers: { 'Content-Type': 'application/json' },
+                    body: JSON.stringify({
+                        event: webhook.event,
+                        data: {
+                            ...webhook.data,
+                            createdAt: new Date().toISOString(),
+                        },
+                    }),
+                });
+                console.log('Sent webhook');
+            } catch (e) {
+                console.error('Error sending webhook', e);
+            }
+        })
+    );
lib/lens-frames.ts (2)

35-43: Handle invalid Lens frame messages appropriately

In the validation check starting at line 38, a generic error message 'Invalid Lens frame message' is returned. Providing more specific error details can help with debugging and user feedback.

Consider returning the specific error from lensValidation:

if (!lensValidation.valid) {
    return {
        isValid: false,
-       error: 'Invalid Lens frame message',
+       error: lensValidation.error || 'Invalid Lens frame message',
    };
}

77-77: Optional chaining for request.untrustedData.state

On line 77, you access request.untrustedData.state without checking if untrustedData is defined. Although it's unlikely in this context, using optional chaining can prevent potential runtime errors.

Modify the line as follows:

- serialized: request.untrustedData.state || '',
+ serialized: request.untrustedData.state ?? '',

Alternatively, ensure that untrustedData is always defined or add guards if necessary.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between e5ddde4 and f497697.

📒 Files selected for processing (22)
  • app/(app)/f/[frameId]/[handler]/[[...query]]/route.ts (2 hunks)
  • app/(app)/f/[frameId]/interactions/route.ts (1 hunks)
  • app/(app)/f/[frameId]/show/page.tsx (1 hunks)
  • app/(app)/loading.tsx (0 hunks)
  • app/(app)/of/[frameId]/[handler]/[[query]]/route.ts (1 hunks)
  • app/(app)/p/[frameId]/[handler]/[[...query]]/route.ts (2 hunks)
  • app/(app)/page.tsx (1 hunks)
  • app/(app)/templates/page.tsx (1 hunks)
  • app/layout.tsx (0 hunks)
  • app/loading.tsx (0 hunks)
  • app/manifest.ts (1 hunks)
  • app/robots.ts (1 hunks)
  • lib/farcaster.d.ts (2 hunks)
  • lib/frame.ts (1 hunks)
  • lib/lens-frames.ts (1 hunks)
  • lib/openframes.ts (1 hunks)
  • lib/serve.ts (5 hunks)
  • lib/storage.ts (1 hunks)
  • lib/template.ts (1 hunks)
  • lib/types.d.ts (2 hunks)
  • lib/validation.ts (1 hunks)
  • package.json (1 hunks)
💤 Files with no reviewable changes (3)
  • app/(app)/loading.tsx
  • app/layout.tsx
  • app/loading.tsx
✅ Files skipped from review due to trivial changes (6)
  • app/(app)/f/[frameId]/interactions/route.ts
  • app/(app)/page.tsx
  • app/(app)/templates/page.tsx
  • app/manifest.ts
  • app/robots.ts
  • lib/storage.ts
🧰 Additional context used
🪛 Biome
app/(app)/f/[frameId]/[handler]/[[...query]]/route.ts

[error] 1-1: All these imports are only used as types.

Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.

(lint/style/useImportType)

app/(app)/of/[frameId]/[handler]/[[query]]/route.ts

[error] 1-1: All these imports are only used as types.

Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.

(lint/style/useImportType)

app/(app)/p/[frameId]/[handler]/[[...query]]/route.ts

[error] 1-1: All these imports are only used as types.

Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.

(lint/style/useImportType)


[error] 9-10: All these imports are only used as types.

Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.

(lint/style/useImportType)

lib/lens-frames.ts

[error] 1-1: All these imports are only used as types.

Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.

(lint/style/useImportType)


[error] 50-50: Use Number.parseInt instead of the equivalent global.

ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.

(lint/style/useNumberNamespace)


[error] 82-82: Use Number.parseInt instead of the equivalent global.

ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.

(lint/style/useNumberNamespace)


[error] 85-85: Use Number.parseInt instead of the equivalent global.

ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.

(lint/style/useNumberNamespace)


[error] 118-118: Use Number.parseInt instead of the equivalent global.

ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.

(lint/style/useNumberNamespace)

lib/openframes.ts

[error] 1-1: All these imports are only used as types.

Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.

(lint/style/useImportType)

lib/serve.ts

[error] 227-231: Prefer for...of instead of forEach.

forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.

(lint/complexity/noForEach)

lib/validation.ts

[error] 10-10: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

The declaration is defined in this switch clause:

Unsafe fix: Wrap the declaration in a block.

(lint/correctness/noSwitchDeclarations)


[error] 16-16: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

The declaration is defined in this switch clause:

Unsafe fix: Wrap the declaration in a block.

(lint/correctness/noSwitchDeclarations)


[error] 1-2: Some named imports are only used as types.

This import is only used as a type.

Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.

(lint/style/useImportType)


[error] 2-3: Some named imports are only used as types.

This import is only used as a type.

This import is only used as a type.

Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.

(lint/style/useImportType)

🔇 Additional comments (18)
package.json (1)

1-118: Summary: Package.json changes align well with PR objectives.

The modifications to package.json are consistent with the PR objectives:

  1. The addition of @lens-protocol/client supports the implementation of lens frames.
  2. The inclusion of @xmtp/frames-validator enables XMTP support and adds validation capabilities.
  3. The introduction of Biome for linting, formatting, and checking enhances code quality and consistency.

These changes collectively contribute to improving the project's functionality and maintainability. The suggestions provided in the previous comments can further refine these improvements.

lib/openframes.ts (2)

25-45: 'buildOpenFrameMetadata' function implementation looks good

The buildOpenFrameMetadata function correctly constructs an OpenFrameMetadata object from buildFrameData. The mapping and assignments align with the interface definitions, and the implementation appears sound.


35-39: ⚠️ Potential issue

Include 'postUrl' in button mapping if applicable

In the buildOpenFrameMetadata function, the postUrl property is defined as optional in the OpenFrameButton interface (line 21) but is not included in the mapping of buildFrameData.buttons.

Apply this diff to include postUrl:

 buttons: buildFrameData.buttons?.map((button) => ({
     label: button.label,
     action: button.action as 'post' | 'post_redirect' | 'link' | 'mint' | 'tx',
     target: button.target,
+    postUrl: button.postUrl,
 })),

Please verify whether postUrl should be included based on the button specifications in buildFrameData.

lib/validation.ts (1)

28-28: Ensure returning untrusted data directly is safe in 'anonymous' case

Returning payload.untrustedData without validation may pose security risks, especially if the data originates from an untrusted source. Please verify that it's safe to return this data directly and that any necessary validation occurs elsewhere.

Run the following script to check where payload.untrustedData is used and if there are validation mechanisms in place:

lib/template.ts (6)

5-16: Well-defined Template type enhances type safety and code maintainability

The addition of the Template type provides a clear structure for template objects and improves type safety across the codebase.


20-20: Confirm the inclusion of 'podcast' and 'nft' in FEATURED_IDS

The addition of 'podcast' and 'nft' to FEATURED_IDS will feature these templates prominently. Please verify that this aligns with the product requirements and that these templates are ready for featuring.


36-37: Review the impact of changing revalidate period to 1 minute

Reducing the cache revalidation time from one day to one minute will increase the frequency of cache refreshes. This may impact performance or increase load on the server. Confirm that this change is intentional and aligns with caching strategy requirements.


53-54: Verify the decrease in revalidate period to 1 minute

Confirm that changing the cache revalidate period from one day to one minute is appropriate for the application's performance and caching requirements.


59-64: Improved type safety with generic parameter in getTemplate function

Using the generic type <T extends keyof typeof templates> ensures that the id parameter is a valid key, which enhances type safety.


68-69: Assess the effect of reducing revalidate period to 1 minute

Ensure that the change in cache revalidation frequency is intentional and fits within the overall caching strategy.

lib/farcaster.d.ts (3)

52-52: Approved: Addition of openFrameMetadata property

The inclusion of the optional openFrameMetadata?: OpenFrameMetadata property in the BuildFrameData interface is appropriate and enhances flexibility in building frame data with open frame metadata.


104-104: Approved: Definition of ClientProtocol type

The ClientProtocol type is appropriately defined, clearly specifying the allowed client protocols.


109-116: Approved: Extension of untrustedData properties

The expansion of untrustedData in the FrameRequest interface with additional optional properties (inputText, state, address, transactionId) enhances the flexibility and comprehensiveness of request data handling.

app/(app)/p/[frameId]/[handler]/[[...query]]/route.ts (1)

47-47: Ensure validateFramePayload supports both request types

The payload variable is cast to FrameRequest | LensFrameRequest, but validateFramePayload may not handle LensFrameRequest. Please verify that validateFramePayload can validate both FrameRequest and LensFrameRequest types.

Run the following script to check if validateFramePayload supports LensFrameRequest:

✅ Verification successful

Validation of LensFrameRequest in validateFramePayload is handled appropriately

  • validateFramePayload accepts FrameRequest and delegates validation for LensFrameRequest to validateLensFrame.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for type annotations of validateFramePayload to confirm it accepts LensFrameRequest.

# Test: Find the function definition. Expect: The function accepts both FrameRequest and LensFrameRequest.
rg --type typescript -A 5 'function validateFramePayload\('

Length of output: 96


Script:

#!/bin/bash
# Description: Search for type annotations of validateFramePayload to confirm it accepts LensFrameRequest.

# Test: Find the function definition. Expect: The function accepts both FrameRequest and LensFrameRequest.
rg --type ts -A 5 'function validateFramePayload\('

Length of output: 495

lib/types.d.ts (1)

149-149: Verify the validity of 'versions' in the Octicon type

The addition of 'versions' to the Octicon type suggests a new icon. Please confirm that 'versions' is a valid Octicon icon name to prevent potential runtime errors or missing icon issues.

Run the following script to check if 'versions' is a valid Octicon icon:

app/(app)/f/[frameId]/[handler]/[[...query]]/route.ts (1)

160-166: Ensure optional properties are handled safely

When inserting into the interactionTable, some properties like inputText, state, transactionHash, castFid, and castHash are accessed from validatedPayload. Ensure that these properties are present or handle them appropriately to prevent potential runtime errors if they are undefined.

If you are confident that validateFramePayload guarantees the presence of these properties, no action is needed. Otherwise, consider adding checks or defaults.

lib/serve.ts (2)

75-92: Construction of openFrameMetadataFinal is correct

The openFrameMetadataFinal object is properly constructed, consolidating various properties such as version, accepts, image, buttons, postUrl, inputText, and imageAspectRatio. This ensures that the metadata is comprehensive and accurately reflects the required information.


221-254: ⚠️ Potential issue

Misplaced return statement causing unreachable code in buildFrame function

The return metadata statement at line 253~ occurs before important code that initializes the metadata object starting from line 261~ and performs version validation at line 256~. This means that the code after return metadata is unreachable, and the metadata object may not be properly constructed, leading to incorrect behavior.

Apply this diff to fix the issue by moving the return metadata statement after the metadata initialization and version validation:

        // ... existing code ...
-    return metadata
            // ... existing code ...
        // ... existing code ...
+    return metadata

Likely invalid or redundant comment.

🧰 Tools
🪛 Biome

[error] 227-231: Prefer for...of instead of forEach.

forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.

(lint/complexity/noForEach)

@@ -0,0 +1,45 @@
import { BuildFrameData } from "./farcaster"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use 'import type' for type-only imports to optimize build

The import on line 1 is used only for typing purposes. Using import type helps TypeScript and bundlers to remove type-only imports during transpilation, improving build performance.

Apply this diff to fix the issue:

-import { BuildFrameData } from "./farcaster"
+import type { BuildFrameData } from "./farcaster"
📝 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.

Suggested change
import { BuildFrameData } from "./farcaster"
import type { BuildFrameData } from "./farcaster"
🧰 Tools
🪛 Biome

[error] 1-1: All these imports are only used as types.

Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.

(lint/style/useImportType)

Comment on lines +1 to +3
import { validateFramesPost } from '@xmtp/frames-validator';
import { LensFrameRequest, validateLensFrame } from './lens-frames';
import { FramePayloadValidated, FrameRequest, validatePayload as validateFarcasterPayload } from './farcaster';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use import type for type-only imports to optimize build

Some imported types are only used for type annotations and can be safely imported using import type. This helps in tree-shaking and reduces the bundle size by ensuring these imports are removed during transpilation.

Apply this diff to update type-only imports:

-import { validateFramesPost } from '@xmtp/frames-validator';
-import { LensFrameRequest, validateLensFrame } from './lens-frames';
-import { FramePayloadValidated, FrameRequest, validatePayload as validateFarcasterPayload } from './farcaster';
+import type { FramePayloadValidated, FrameRequest } from './farcaster';
+import { validatePayload as validateFarcasterPayload } from './farcaster';
+import type { LensFrameRequest } from './lens-frames';
+import { validateLensFrame } from './lens-frames';
+import { validateFramesPost } from '@xmtp/frames-validator';
📝 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.

Suggested change
import { validateFramesPost } from '@xmtp/frames-validator';
import { LensFrameRequest, validateLensFrame } from './lens-frames';
import { FramePayloadValidated, FrameRequest, validatePayload as validateFarcasterPayload } from './farcaster';
import type { FramePayloadValidated, FrameRequest } from './farcaster';
import { validatePayload as validateFarcasterPayload } from './farcaster';
import type { LensFrameRequest } from './lens-frames';
import { validateLensFrame } from './lens-frames';
import { validateFramesPost } from '@xmtp/frames-validator';
🧰 Tools
🪛 Biome

[error] 1-2: Some named imports are only used as types.

This import is only used as a type.

Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.

(lint/style/useImportType)


[error] 2-3: Some named imports are only used as types.

This import is only used as a type.

This import is only used as a type.

Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.

(lint/style/useImportType)

Comment on lines +10 to +14
const lensValidation = await validateLensFrame(payload as LensFrameRequest);
if (!lensValidation.isValid || !lensValidation.validatedPayload) {
throw new Error(lensValidation.error || 'Invalid Lens frame message');
}
return lensValidation.validatedPayload;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Wrap 'lens' case block in braces to restrict variable scope

Variables declared inside a switch case are hoisted and accessible in other cases due to JavaScript's function-level scoping. To prevent potential variable leakage and scope issues, it's best to wrap the case block in braces {}.

Apply this diff to wrap the 'lens' case block:

        case 'lens':
+        {
            const lensValidation = await validateLensFrame(payload as LensFrameRequest);
            if (!lensValidation.isValid || !lensValidation.validatedPayload) {
                throw new Error(lensValidation.error || 'Invalid Lens frame message');
            }
            return lensValidation.validatedPayload;
+        }
📝 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.

Suggested change
const lensValidation = await validateLensFrame(payload as LensFrameRequest);
if (!lensValidation.isValid || !lensValidation.validatedPayload) {
throw new Error(lensValidation.error || 'Invalid Lens frame message');
}
return lensValidation.validatedPayload;
case 'lens':
{
const lensValidation = await validateLensFrame(payload as LensFrameRequest);
if (!lensValidation.isValid || !lensValidation.validatedPayload) {
throw new Error(lensValidation.error || 'Invalid Lens frame message');
}
return lensValidation.validatedPayload;
}
🧰 Tools
🪛 Biome

[error] 10-10: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

The declaration is defined in this switch clause:

Unsafe fix: Wrap the declaration in a block.

(lint/correctness/noSwitchDeclarations)

Comment on lines +16 to +26
const xmtpValidation = await validateFramesPost(payload);
if (!xmtpValidation.valid) {
throw new Error('Invalid XMTP frame message');
}
return {
...payload.untrustedData,
interactor: {
fid: 0,
username: xmtpValidation.verifiedWalletAddress,
},
} as FramePayloadValidated;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Wrap 'xmtp' case block in braces to restrict variable scope

Similar to the 'lens' case, variables declared here can leak into other cases. Wrapping the 'xmtp' case block in braces ensures variables like xmtpValidation are scoped correctly.

Apply this diff to wrap the 'xmtp' case block:

        case 'xmtp':
+        {
            const xmtpValidation = await validateFramesPost(payload);
            if (!xmtpValidation.valid) {
                throw new Error('Invalid XMTP frame message');
            }
            return {
                ...payload.untrustedData,
                interactor: {
                    fid: 0,
                    username: xmtpValidation.verifiedWalletAddress,
                },
            } as FramePayloadValidated;
+        }
📝 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.

Suggested change
const xmtpValidation = await validateFramesPost(payload);
if (!xmtpValidation.valid) {
throw new Error('Invalid XMTP frame message');
}
return {
...payload.untrustedData,
interactor: {
fid: 0,
username: xmtpValidation.verifiedWalletAddress,
},
} as FramePayloadValidated;
case 'xmtp':
{
const xmtpValidation = await validateFramesPost(payload);
if (!xmtpValidation.valid) {
throw new Error('Invalid XMTP frame message');
}
return {
...payload.untrustedData,
interactor: {
fid: 0,
username: xmtpValidation.verifiedWalletAddress,
},
} as FramePayloadValidated;
}
🧰 Tools
🪛 Biome

[error] 16-16: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.

The declaration is defined in this switch clause:

Unsafe fix: Wrap the declaration in a block.

(lint/correctness/noSwitchDeclarations)

Comment on lines +62 to +64
...template,
id: id,
name: template.name,
description: template.description,
shortDescription: template.shortDescription,
icon: template.icon,
cover: template.cover,
creatorFid: template.creatorFid,
creatorName: template.creatorName,
enabled: template.enabled,
requiresValidation: template.requiresValidation,
}
} as unknown as Template
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Eliminate unnecessary type assertions for cleaner code

As with previous functions, the double casting to unknown can be avoided.

Adjust the return statement to directly cast to Template:

            return {
                ...template,
                id: id,
-            } as unknown as Template
+            } as Template
📝 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.

Suggested change
...template,
id: id,
name: template.name,
description: template.description,
shortDescription: template.shortDescription,
icon: template.icon,
cover: template.cover,
creatorFid: template.creatorFid,
creatorName: template.creatorName,
enabled: template.enabled,
requiresValidation: template.requiresValidation,
}
} as unknown as Template
...template,
id: id,
} as Template

Comment on lines +48 to +74
interactor: {
object: 'user',
fid: parseInt(request.untrustedData.profileId),
custody_address: request.trustedData.signer || '',
username: '',
display_name: '',
pfp_url: '',
profile: {
bio: {
text: '',
mentioned_profiles: [],
},
},
follower_count: 0,
following_count: 0,
verifications: [],
verified_addresses: {
eth_addresses: [],
sol_addresses: [],
},
active_status: 'inactive',
power_badge: false,
viewer_context: {
following: false,
followed_by: false,
},
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor duplicated user object creation

The interactor, author, and signer.client objects (lines 48-74, 83-105, and 116-139) have identical structures. Refactoring them into a helper function will reduce code duplication and improve maintainability.

Consider creating a helper function like this:

function createUser(profileId: string, signer: string | undefined): User {
    return {
        object: 'user',
        fid: Number.parseInt(profileId),
        custody_address: signer || '',
        username: '',
        display_name: '',
        pfp_url: '',
        profile: {
            bio: {
                text: '',
                mentioned_profiles: [],
            },
        },
        follower_count: 0,
        following_count: 0,
        verifications: [],
        verified_addresses: {
            eth_addresses: [],
            sol_addresses: [],
        },
        active_status: 'inactive',
        power_badge: false,
        viewer_context: {
            following: false,
            followed_by: false,
        },
    };
}

Then, replace the duplicated objects with calls to createUser:

// For `interactor` at lines 48-74
- interactor: {
-     // ...user object...
- },
+ interactor: createUser(request.untrustedData.profileId, request.trustedData.signer),

// For `author` at lines 83-105
- author: {
-     // ...user object...
- },
+ author: createUser(request.untrustedData.profileId, request.trustedData.signer),

// For `signer.client` at lines 116-139
- client: {
-     // ...user object...
- },
+ client: createUser(request.untrustedData.profileId, request.trustedData.signer),

Also applies to: 83-105, 116-139

🧰 Tools
🪛 Biome

[error] 50-50: Use Number.parseInt instead of the equivalent global.

ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.

(lint/style/useNumberNamespace)

@@ -0,0 +1,158 @@
import { FramePayloadValidated } from './farcaster';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use import type for type-only imports

The import on line 1 is only used for types. Using import type ensures that the type is removed during transpilation, avoiding unnecessary module loading.

Apply this diff to fix the issue:

-import { FramePayloadValidated } from './farcaster';
+import type { FramePayloadValidated } from './farcaster';
📝 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.

Suggested change
import { FramePayloadValidated } from './farcaster';
import type { FramePayloadValidated } from './farcaster';
🧰 Tools
🪛 Biome

[error] 1-1: All these imports are only used as types.

Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.

(lint/style/useImportType)

Comment on lines +227 to +229
Object.entries(openFrameMetadata.accepts).forEach(([protocol, version]) => {
metadata[`of:accepts:${protocol}`] = version
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Replace forEach with for...of for better performance

Using forEach can lead to performance issues when working with large arrays. It's recommended to use a for...of loop instead for improved performance and readability.

Apply this diff to replace the forEach loop:

      + for (const [protocol, version] of Object.entries(openFrameMetadata.accepts)) {
      + }

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 Biome

[error] 227-231: Prefer for...of instead of forEach.

forEach may lead to performance issues when working with large arrays. When combined with functions like filter or map, this causes multiple iterations over the same type.

(lint/complexity/noForEach)

Comment on lines +281 to +306
if (!button.label || button.label.length > 256) {
throw new Error('Button label is required and must be maximum of 256 bytes.')
}
buttons.forEach((button: FrameButtonMetadata, index: number) => {
if (!button.label || button.label.length > 256) {
throw new Error('Button label is required and must be maximum of 256 bytes.')
}
metadata[`fc:frame:button:${index + 1}`] = button.label
metadata[`fc:frame:button:${index + 1}`] = button.label

if (button.action) {
if (!['post', 'post_redirect', 'mint', 'link', 'tx'].includes(button.action)) {
throw new Error('Invalid button action.')
}
metadata[`fc:frame:button:${index + 1}:action`] = button.action

if (button.action === 'tx') {
metadata[`fc:frame:button:${index + 1}:target`] =
postUrl + `/${button.handler || handler}` + '?' + searchParams
if (button.callback) {
metadata[`fc:frame:button:${index + 1}:post_url`] =
postUrl + `/${button.callback}` + '?' + searchParams
}
}
if (button.action === 'link' || button.action === 'mint') {
metadata[`fc:frame:button:${index + 1}:target`] = button.target
if (button.action) {
if (!['post', 'post_redirect', 'mint', 'link', 'tx'].includes(button.action)) {
throw new Error('Invalid button action.')
}
metadata[`fc:frame:button:${index + 1}:action`] = button.action

if (button.action === 'tx') {
metadata[`fc:frame:button:${index + 1}:target`] =
postUrl + `/${button.handler || handler}` + '?' + searchParams
if (button.callback) {
metadata[`fc:frame:button:${index + 1}:post_url`] =
postUrl + `/${button.callback}` + '?' + searchParams
}
} else {
metadata[`fc:frame:button:${index + 1}:action`] = 'post' // Default action
}
})
}

if (refreshPeriod) {
if (refreshPeriod < 0) {
throw new Error('Refresh period must be a positive number.')
if (button.action === 'link' || button.action === 'mint') {
metadata[`fc:frame:button:${index + 1}:target`] = button.target
}
} else {
metadata[`fc:frame:button:${index + 1}:action`] = 'post' // Default action
}
metadata['fc:frame:refresh_period'] = refreshPeriod.toString()
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve loop performance in button handling

In the buttons processing, replacing forEach with for...of enhances performance and maintains consistency.

Apply this diff:

      + for (const [index, button] of buttons.entries()) {
            throw new Error('Button label is required and must be maximum of 256 bytes.')
        }
            // ... existing code ...

Committable suggestion was skipped due to low confidence.

Comment on lines +232 to +237
openFrameMetadata.buttons.forEach((button, index) => {
metadata[`of:button:${index + 1}`] = button.label
if (button.action) metadata[`of:button:${index + 1}:action`] = button.action
if (button.target) metadata[`of:button:${index + 1}:target`] = button.target
if (button.postUrl) metadata[`of:button:${index + 1}:post_url`] = button.postUrl
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimize button processing by replacing forEach with for...of

Similarly, replacing the forEach loop with a for...of loop in the button processing enhances performance.

Apply this diff:

      + for (const [index, button] of openFrameMetadata.buttons.entries()) {

Committable suggestion was skipped due to low confidence.

@developerfred developerfred changed the title [FH-2] Open Frames Implementation #163 [FH-2] Open Frames Implementation (#163) Oct 4, 2024
@FTCHD
Copy link
Owner

FTCHD commented Oct 4, 2024

  1. your linter/prettier changed many files which should not be part of this PR, it also added semicolons ";" everywhere. this does not conform to our Biome config so please install the extension and make sure all extra files/changes are removed from this PR (by committing a new version that's the same as the initial before your linter did the changes).
  2. did you test the /of route somewhere? If not then please do and post the link here to the Lens post.

ty and lfg

@FTCHD FTCHD closed this Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants