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] 8 Ball (#160) #178

Merged
merged 3 commits into from
Oct 21, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions templates/eightball/Inspector.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
'use client'
import { BasicViewInspector, GatingInspector, Switch } from '@/sdk/components'
import { useFrameConfig } from '@/sdk/hooks'
import { Configuration } from '@/sdk/inspector'
import type { Config } from '.'

type GatingConfig = Required<Config['gating']>

export default function Inspector() {
const [config, updateConfig] = useFrameConfig<Config>()

const gatingConfig: GatingConfig = config.gating || {
enabled: [],
requirements: {
maxFid: 0,
minFid: 0,
score: 0,
channels: [],
exactFids: [],
erc20: null,
erc721: null,
erc1155: null,
moxie: null,
},
}
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

Consider optimizing gatingConfig object creation.

The gatingConfig object is currently defined inside the component, which means it will be recreated on every render. This could potentially impact performance, especially if the component re-renders frequently.

Consider moving the gatingConfig object outside the component or memoizing it using useMemo to optimize performance:

const defaultGatingConfig: GatingConfig = {
  enabled: [],
  requirements: {
    maxFid: 0,
    minFid: 0,
    score: 0,
    channels: [],
    exactFids: [],
    erc20: null,
    erc721: null,
    erc1155: null,
    moxie: null,
  },
};

export default function Inspector() {
  const [config, updateConfig] = useFrameConfig<Config>();

  const gatingConfig = useMemo(() => ({
    ...defaultGatingConfig,
    ...config.gating,
  }), [config.gating]);

  // Rest of the component...
}

This approach ensures that the gatingConfig object is only recreated when config.gating changes.


return (
<Configuration.Root>
<Configuration.Section title="General">
<label>
Cooldown (seconds):
<input
type="number"
value={config.cooldown}
onChange={(e) =>
updateConfig({ cooldown: Number.parseInt(e.target.value) || -1 })
}
min="-1"
/>
</label>
<label>
Public:
<input
type="checkbox"
checked={config.isPublic}
onChange={(e) => updateConfig({ isPublic: e.target.checked })}
/>
</label>
</Configuration.Section>
<Configuration.Section title="Cover">
<BasicViewInspector
config={config.coverConfig}
onChange={(newCoverConfig) => updateConfig({ coverConfig: newCoverConfig })}
/>
</Configuration.Section>
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

Add validation for cooldown input.

The cooldown input currently allows negative values, which might not be the intended behavior for a cooldown setting.

Consider adding validation to ensure the cooldown value is non-negative:

 <input
     type="number"
     value={config.cooldown}
     onChange={(e) =>
-        updateConfig({ cooldown: Number.parseInt(e.target.value) || -1 })
+        updateConfig({ cooldown: Math.max(0, Number.parseInt(e.target.value) || 0) })
     }
-    min="-1"
+    min="0"
 />

This change ensures that the cooldown value is always non-negative, with a minimum of 0 seconds.

📝 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
return (
<Configuration.Root>
<Configuration.Section title="General">
<label>
Cooldown (seconds):
<input
type="number"
value={config.cooldown}
onChange={(e) =>
updateConfig({ cooldown: Number.parseInt(e.target.value) || -1 })
}
min="-1"
/>
</label>
<label>
Public:
<input
type="checkbox"
checked={config.isPublic}
onChange={(e) => updateConfig({ isPublic: e.target.checked })}
/>
</label>
</Configuration.Section>
<Configuration.Section title="Cover">
<BasicViewInspector
config={config.coverConfig}
onChange={(newCoverConfig) => updateConfig({ coverConfig: newCoverConfig })}
/>
</Configuration.Section>
return (
<Configuration.Root>
<Configuration.Section title="General">
<label>
Cooldown (seconds):
<input
type="number"
value={config.cooldown}
onChange={(e) =>
updateConfig({ cooldown: Math.max(0, Number.parseInt(e.target.value) || 0) })
}
min="0"
/>
</label>
<label>
Public:
<input
type="checkbox"
checked={config.isPublic}
onChange={(e) => updateConfig({ isPublic: e.target.checked })}
/>
</label>
</Configuration.Section>
<Configuration.Section title="Cover">
<BasicViewInspector
config={config.coverConfig}
onChange={(newCoverConfig) => updateConfig({ coverConfig: newCoverConfig })}
/>
</Configuration.Section>

<Configuration.Section title="Answer">
<BasicViewInspector
config={config.answerConfig}
onChange={(newAnswerConfig) => updateConfig({ answerConfig: newAnswerConfig })}
/>
</Configuration.Section>
<Configuration.Section title="Gating">
<Switch
checked={config.enableGating ?? false}
onCheckedChange={(checked) => updateConfig({ enableGating: checked })}
/>
{config.enableGating && (
<GatingInspector
config={gatingConfig}
onChange={(newGatingConfig) => updateConfig({ gating: newGatingConfig })}
/>
)}
</Configuration.Section>
</Configuration.Root>
)
}
Binary file added templates/eightball/cover.jpeg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
81 changes: 81 additions & 0 deletions templates/eightball/handlers/ask.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
'use server'
import type { BuildFrameData } from '@/lib/farcaster'
import { runGatingChecks } from '@/lib/gating'
import { loadGoogleFontAllVariants } from '@/sdk/fonts'
import {
type ButtonConfig,
type ErrorDisplayConfig,
type HandlerContext,
Magic8BallError,
checkCooldown,
getRandomAnswer,
validateQuestion,
} from '../types'
import AnswerView from '../views/answer'

const errorDisplayConfig: ErrorDisplayConfig = {
style: {
backgroundColor: 'red',
color: 'white',
padding: '20px',
borderRadius: '10px',
textAlign: 'center',
fontSize: '24px',
},
buttons: [{ label: 'Try Again' }],
handler: 'initial',
}
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

Extend ErrorDisplayConfig to accept dynamic error content

To display specific error messages to the user, ErrorDisplayConfig should be extended to include an errorContent property or similar.

Update the errorDisplayConfig definition:

 const errorDisplayConfig: ErrorDisplayConfig = {
     style: {
         backgroundColor: 'red',
         color: 'white',
         padding: '20px',
         borderRadius: '10px',
         textAlign: 'center',
         fontSize: '24px',
     },
     buttons: [{ label: 'Try Again' }],
     handler: 'initial',
+    errorContent: '', // Placeholder for dynamic error messages
 }
📝 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 errorDisplayConfig: ErrorDisplayConfig = {
style: {
backgroundColor: 'red',
color: 'white',
padding: '20px',
borderRadius: '10px',
textAlign: 'center',
fontSize: '24px',
},
buttons: [{ label: 'Try Again' }],
handler: 'initial',
}
const errorDisplayConfig: ErrorDisplayConfig = {
style: {
backgroundColor: 'red',
color: 'white',
padding: '20px',
borderRadius: '10px',
textAlign: 'center',
fontSize: '24px',
},
buttons: [{ label: 'Try Again' }],
handler: 'initial',
errorContent: '', // Placeholder for dynamic error messages
}


const createButton = (label: string): ButtonConfig => ({ label })

const updateStorage = (
storage: HandlerContext['storage'],
body: HandlerContext['body'],
answer: string
) => ({
...storage,
lastAnswerTime: Date.now(),
questions: [
...(storage.questions || []),
{
question: body.inputText,
answer,
timestamp: Date.now(),
fid: body.fid,
},
],
})
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 a single timestamp for consistency

Currently, Date.now() is called multiple times in updateStorage(), which can lead to slight inconsistencies in timestamps. Capturing the timestamp once ensures that all related time fields are consistent.

Apply this diff to capture Date.now() once:

 const updateStorage = (
     storage: HandlerContext['storage'],
     body: HandlerContext['body'],
     answer: string
 ) => {
+    const timestamp = Date.now()
     return {
         ...storage,
-        lastAnswerTime: Date.now(),
+        lastAnswerTime: timestamp,
         questions: [
             ...(storage.questions || []),
             {
                 question: body.inputText,
                 answer,
-                timestamp: Date.now(),
+                timestamp,
                 fid: body.fid,
             },
         ],
     }
 }
📝 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 updateStorage = (
storage: HandlerContext['storage'],
body: HandlerContext['body'],
answer: string
) => ({
...storage,
lastAnswerTime: Date.now(),
questions: [
...(storage.questions || []),
{
question: body.inputText,
answer,
timestamp: Date.now(),
fid: body.fid,
},
],
})
const updateStorage = (
storage: HandlerContext['storage'],
body: HandlerContext['body'],
answer: string
) => {
const timestamp = Date.now()
return {
...storage,
lastAnswerTime: timestamp,
questions: [
...(storage.questions || []),
{
question: body.inputText,
answer,
timestamp,
fid: body.fid,
},
],
}
}


export default async function ask({
body,
config,
storage,
}: HandlerContext): Promise<BuildFrameData> {
try {
const roboto = await loadGoogleFontAllVariants('Roboto')

runGatingChecks(body, config.gating)
validateQuestion(body.inputText)
checkCooldown(storage.lastAnswerTime, config.cooldown)

const answer = getRandomAnswer()
const newStorage = updateStorage(storage, body, answer)

return {
buttons: [createButton('Ask Another Question')],
fonts: roboto,
component: AnswerView(config, answer),
handler: 'initial',
storage: newStorage,
}
} catch (error) {
if (error instanceof Magic8BallError) {
return {
...errorDisplayConfig,
// Assuming you might want some error message or dynamic content here
// errorContent: error.message,
}
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

Include error messages in user-facing error responses

When catching a Magic8BallError, the specific error message is not included in the response. Including error.message can provide users with helpful feedback.

Apply this diff to include the error message:

 return {
     ...errorDisplayConfig,
+    errorContent: error.message,
 }

Ensure that ErrorDisplayConfig accepts an errorContent property.

📝 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
return {
...errorDisplayConfig,
// Assuming you might want some error message or dynamic content here
// errorContent: error.message,
}
return {
...errorDisplayConfig,
errorContent: error.message,
}

}
throw error
}
}
3 changes: 3 additions & 0 deletions templates/eightball/handlers/handlers.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export { default as initial } from './initial';
export { default as page } from './page';
export { default as ask } from './ask';
7 changes: 7 additions & 0 deletions templates/eightball/handlers/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { initial, ask, page} from './handlers';

export default {
initial,
ask,
page
}
20 changes: 20 additions & 0 deletions templates/eightball/handlers/initial.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
'use server'
import type { BuildFrameData } from '@/lib/farcaster'
import { loadGoogleFontAllVariants } from '@/sdk/fonts'
import type { HandlerContext } from '../types'
import CoverView from '../views/cover'

export default async function initial({
config,
storage,
}: Omit<HandlerContext, 'body'>): Promise<BuildFrameData> {
const roboto = await loadGoogleFontAllVariants('Roboto')

return {
buttons: [{ label: 'Ask' }],
inputText: 'Ask a question...',
fonts: roboto,
component: CoverView(config),
handler: 'ask',
}
}
30 changes: 30 additions & 0 deletions templates/eightball/handlers/page.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
'use server'
import type { BuildFrameData, FramePayloadValidated } from '@/lib/farcaster'
import { loadGoogleFontAllVariants } from '@/sdk/fonts'
import type { Config, Storage } from '..'
import PageView from '../views/page'

export default async function page({
body,
config,
storage,
params,
}: {
body: FramePayloadValidated
config: Config
storage: Storage
params: any
}): Promise<BuildFrameData> {
const roboto = await loadGoogleFontAllVariants('Roboto')
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

Consider optimizing font loading

Loading all variants of the Roboto font might be unnecessary and could impact performance. Consider loading only the specific font weights and styles required for this component.

You could modify the font loading as follows:

const roboto = await loadGoogleFontAllVariants('Roboto', ['400', '700']); // Example: load only regular and bold weights

Ensure to update the loadGoogleFontAllVariants function to accept an optional array of weights/styles if it doesn't already.


return {
buttons: [
{
label: '← Back to Magic 8 Ball',
},
],
fonts: roboto,
component: PageView(config, storage, params),
handler: 'initial',
}
}
89 changes: 89 additions & 0 deletions templates/eightball/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import type { BaseConfig, BaseStorage, BaseTemplate } from '@/lib/types'
import type { GatingType } from '@/sdk/components/gating/types'
import Inspector from './Inspector'
import cover from './cover.jpeg'
import handlers from './handlers'

export interface Config extends BaseConfig {
fontFamily?: string
primaryColor?: string
secondaryColor?: string
titleWeight?: string
titleStyle?: string
background?: string
bodyColor?: string
cooldown: number
isPublic: boolean
coverConfig: {
title: string
subtitle: string
bottomMessage: string
backgroundColor: string
textColor: string
}
answerConfig: {
title: string
bottomMessage: string
backgroundColor: string
textColor: string
}
gating: GatingType | undefined
enableGating: boolean | undefined
}
developerfred marked this conversation as resolved.
Show resolved Hide resolved

export interface Storage extends BaseStorage {
lastAnswerTime?: number
questions?: Array<{
question: string
answer: string
timestamp: number
fid: number
}>
}

export default {
name: 'Magic 8 Ball',
description: 'Ask a question and receive a mystical answer from the Magic 8 Ball!',
shortDescription: 'Virtual fortune teller',
icon: '',
octicon: 'crystal-ball',
creatorFid: '197049',
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

Ensure consistent data type for creatorFid

As mentioned in a previous review, the creatorFid is defined as a string, but it's typically used as a number in other parts of the codebase (e.g., in the Storage interface). For consistency and to prevent potential type issues, consider changing creatorFid to a number.

Apply this diff to update creatorFid:

-    creatorFid: '197049',
+    creatorFid: 197049,
📝 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
creatorFid: '197049',
creatorFid: 197049,

creatorName: 'codingsh',
cover,
enabled: true,
Inspector,
handlers,
initialConfig: {
cooldown: 60,
isPublic: true,
coverConfig: {
title: 'Magic 8 Ball',
subtitle: 'Ask a question and receive mystical guidance!',
bottomMessage: "Tap 'Ask' to begin",
backgroundColor: '#000000',
textColor: '#FFFFFF',
},
answerConfig: {
title: 'The 8 Ball says...',
bottomMessage: 'Ask another question to play again!',
backgroundColor: '#000000',
textColor: '#FFFFFF',
},
enableGating: false,
gating: {
enabled: [],
requirements: {
maxFid: 0,
minFid: 0,
score: 0,
channels: [],
exactFids: [],
erc20: null,
erc721: null,
erc1155: null,
moxie: null,
},
},
},
events: ['question.asked', 'question.answered'],
} satisfies BaseTemplate
Loading