-
Notifications
You must be signed in to change notification settings - Fork 777
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
feat: convert reddit pipe to next js #1055
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
nice!
|
i've added the the next cron time component and the test button |
@tribhuwan-kumar nice! i dont see the pipe.json file though ? also content should be {
"crons": [
{
"path": "/api/logpipeline",
"schedule": "0 */1 * * * *"
}
]
} since we now store the settings in the |
i'm writing a single config file which is pipe.json and its sync between app's settings (store.bin) the reddit {
"interval": 60,
"pageSize": 100000,
"summaryFrequency": "daily",
"emailAddress": "mail@com",
"emailPassword": "password",
"emailTime": "11:00",
"customPrompt": "daily",
"dailylogPrompt": "hello",
"contentType": "all",
"windowName": "windows",
"aiUrl": "https://models.inference.ai.azure.com/",
"aiModel": "gpt-4o-mini",
"openaiApiKey": "ghp_OILya30eZ****************IuUe8",
"crons": [
{
"path": "/api/logpipeline",
"schedule": "0 */1 * * * *"
}
]
} at frontend cron time is in seconds, so i'm just converting it minutes. |
sure, i'll do it in another pr
then we can simply remove the {
"crons": [
{
"path": "/api/pipeline",
"schedule": "0 */1 * * * *"
}
],
"pageSize": 100000,
"summaryFrequency": "daily",
"emailAddress": "",
"emailPassword": "",
"emailTime": "11:00",
"customPrompt": "dummy",
"dailylogPrompt": "daily",
"contentType": "all",
"windowName": "windows hello",
"aiUrl": "https://models.inference.ai.azure.com/",
"aiModel": "gpt-4o-mini",
"openaiApiKey": "ghp_***************************e8"
} now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Nitpick comments (3)
pipes/reddit-auto-posts/app/api/pipeline/route.ts (1)
125-131
: Extract screen data querying logic to reduce duplication.The screen data querying logic is duplicated in multiple places.
Apply this diff to extract the common logic:
+async function queryScreenData(params: { + startTime: string; + endTime: string; + windowName: string; + pageSize: number; + contentType: string; +}) { + return retry(() => pipe.queryScreenpipe(params)); +} - const screenData = await retry(() => pipe.queryScreenpipe({ + const screenData = await queryScreenData({ startTime: startTime.toISOString(), endTime: now.toISOString(), windowName: windowName, limit: pageSize, contentType: contentType, - })); + });Also applies to: 180-186
pipes/reddit-auto-posts/components/markdown.tsx (1)
4-9
: Consider all props in memo comparison function.The current implementation only compares
children
andclassName
, ignoring other props from theOptions
type. This could lead to unnecessary re-renders if other props change.export const MemoizedReactMarkdown: FC<Options> = memo( ReactMarkdown, - (prevProps, nextProps) => - prevProps.children === nextProps.children && - prevProps.className === nextProps.className + (prevProps, nextProps) => { + const relevantProps = ['children', 'className', 'remarkPlugins', 'components'] as const; + return relevantProps.every(prop => prevProps[prop] === nextProps[prop]); + } )pipes/reddit-auto-posts/components/pipe.tsx (1)
291-320
: Improve markdown rendering container.The markdown container has a fixed width that might cause layout issues on different screen sizes.
- className="prose break-words dark:prose-invert prose-p:leading-relaxed prose-pre:p-0 w-[35vw] text-sm" + className="prose break-words dark:prose-invert prose-p:leading-relaxed prose-pre:p-0 max-w-full text-sm"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pipes/reddit-auto-posts/bun.lockb
is excluded by!**/bun.lockb
📒 Files selected for processing (4)
pipes/reddit-auto-posts/app/api/pipeline/route.ts
(1 hunks)pipes/reddit-auto-posts/components/markdown.tsx
(1 hunks)pipes/reddit-auto-posts/components/pipe.tsx
(1 hunks)pipes/reddit-auto-posts/package.json
(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
pipes/reddit-auto-posts/app/api/pipeline/route.ts
[error] 134-134: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 188-188: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test-windows
- GitHub Check: test-macos
- GitHub Check: test-linux
- GitHub Check: test-ubuntu
- GitHub Check: test-windows
🔇 Additional comments (10)
pipes/reddit-auto-posts/app/api/pipeline/route.ts (5)
1-10
: LGTM! Well-organized imports following Next.js conventions.The imports are properly structured with a clear separation between built-in Node.js modules and custom application modules.
158-177
:⚠️ Potential issuePersist 'lastEmailSent' to accurately track email schedules.
The
lastEmailSent
variable resets on each function call, causing the email schedule logic to malfunction.Apply this diff to persist the email schedule:
+async function getLastEmailSent(): Promise<Date> { + const screenpipeDir = process.env.SCREENPIPE_DIR || process.cwd(); + const stateFile = path.join(screenpipeDir, "pipes", "reddit-auto-posts", "state.json"); + try { + const state = JSON.parse(await fs.promises.readFile(stateFile, 'utf-8')); + return new Date(state.lastEmailSent); + } catch { + return new Date(0); + } +} + +async function updateLastEmailSent(date: Date): Promise<void> { + const screenpipeDir = process.env.SCREENPIPE_DIR || process.cwd(); + const stateFile = path.join(screenpipeDir, "pipes", "reddit-auto-posts", "state.json"); + await fs.promises.writeFile( + stateFile, + JSON.stringify({ lastEmailSent: date.toISOString() }) + ); +} - let lastEmailSent = new Date(0); + const lastEmailSent = await getLastEmailSent();Likely invalid or redundant comment.
270-276
: 🛠️ Refactor suggestionImprove error handling security and consistency.
The current error handling:
- Exposes internal error details
- Uses inconsistent status codes
- Lacks proper error categorization
Apply this diff to improve error handling:
+const ERROR_MESSAGES = { + INTERNAL_ERROR: 'An internal error occurred', + INVALID_REQUEST: 'Invalid request parameters', + UNAUTHORIZED: 'Unauthorized access', + NOT_FOUND: 'Resource not found', +} as const; + +function sanitizeError(error: unknown): { message: string; status: number } { + console.error('Detailed error:', error); + + if (error instanceof Error) { + if (error.message.includes('access')) { + return { message: ERROR_MESSAGES.UNAUTHORIZED, status: 401 }; + } + if (error.message.includes('not found')) { + return { message: ERROR_MESSAGES.NOT_FOUND, status: 404 }; + } + if (error.message.includes('invalid')) { + return { message: ERROR_MESSAGES.INVALID_REQUEST, status: 400 }; + } + } + return { message: ERROR_MESSAGES.INTERNAL_ERROR, status: 500 }; +} } catch (error) { console.error("error in GET handler:", error); + const { message, status } = sanitizeError(error); return NextResponse.json( - { error: `${error}` }, - { status: 400 } + { error: message }, + { status } ); }Likely invalid or redundant comment.
11-31
: 🛠️ Refactor suggestionImprove log file handling for better reliability and performance.
The current implementation has several areas for improvement:
- Uses synchronous file operations which can block the event loop
- Lacks file size limits
- Basic error handling could be enhanced
Apply this diff to improve the implementation:
async function saveDailyLog(logEntry: DailyLog) { if (!logEntry){ throw new Error("no log entry to save") } console.log("saving log entry:", logEntry); const screenpipeDir = process.env.SCREENPIPE_DIR || process.cwd(); const logsDir = path.join(screenpipeDir, "pipes", "reddit-auto-posts", "logs"); const timestamp = new Date() .toISOString() .replace(/:/g, "-") .replace(/\..+/, ""); const filename = `${timestamp}-${logEntry.category?.replace(/[\/\\?%*:|"<>']/g, "-")}.json`; const logFile = path.join(logsDir, filename) + const logContent = JSON.stringify(logEntry, null, 2); + + // Add size limit (e.g., 1MB) + if (Buffer.byteLength(logContent) > 1024 * 1024) { + throw new Error("Log entry too large"); + } + try { - fs.writeFileSync(logFile, JSON.stringify(logEntry, null, 2)); + await fs.promises.writeFile(logFile, logContent); } catch (error) { console.log(`Failed to write log file: ${error}`) throw new Error(`failed to write log file: ${error}`) } }Likely invalid or redundant comment.
48-82
: 🛠️ Refactor suggestionAdd proper settings validation to prevent runtime errors.
The settings retrieval lacks proper validation and relies on non-null assertions.
Apply this diff to add proper validation:
+interface RedditSettings { + interval: number; + summaryFrequency: string; + emailTime: string; + emailAddress?: string; + emailPassword?: string; + customPrompt: string; + dailylogPrompt: string; + windowName?: string; + pageSize: number; + contentType: string; +} +function validateSettings(settings: unknown): settings is RedditSettings { + if (!settings || typeof settings !== 'object') return false; + const s = settings as Record<string, unknown>; + return ( + typeof s.interval === 'number' && + typeof s.summaryFrequency === 'string' && + typeof s.emailTime === 'string' && + typeof s.customPrompt === 'string' && + typeof s.dailylogPrompt === 'string' && + typeof s.pageSize === 'number' + ); +} export async function GET() { try { const settingsManager = pipe.settings; const redditSettings = await pipe.settings.getNamespaceSettings("reddit-auto-posts"); + if (!validateSettings(redditSettings)) { + return NextResponse.json( + { error: "Invalid settings configuration" }, + { status: 400 } + ); + }Likely invalid or redundant comment.
pipes/reddit-auto-posts/package.json (1)
16-17
: 🛠️ Refactor suggestionSpecify exact versions for internal packages.
Internal packages (@screenpipe/js and @screenpipe/browser) should use exact versions to ensure consistency.
- "@screenpipe/js": "1.0.0", - "@screenpipe/browser": "0.1.12", + "@screenpipe/js": "1.0.0", + "@screenpipe/browser": "0.1.12"Likely invalid or redundant comment.
pipes/reddit-auto-posts/components/pipe.tsx (4)
29-29
: Replaceany
type with proper interface.Using
any
type reduces type safety. Define a proper interface for the log structure.
78-87
: Fix timezone handling ingetNextCronTime
.The function has potential timezone issues that could affect scheduling accuracy.
89-91
: Replace deprecated platform detection.Using
navigator.platform
is deprecated and should be replaced with a more reliable method.
93-125
: Add form validation inhandleSave
.The form submission lacks proper validation for required fields and input formats.
async function retry(fn: any, retries = 3, delay = 5000) { | ||
for (let i = 0; i < retries; i++) { | ||
try { | ||
const result = await fn(); | ||
if (result){ | ||
return result; | ||
} | ||
} catch (error) { | ||
console.log(`Screenpipe query failed, retry, attempt: ${i + 1}`) | ||
if (i === retries - 1) throw error; | ||
await new Promise(res => setTimeout(res, delay)); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add proper type safety to the retry function.
The function uses any
type which bypasses TypeScript's type checking benefits.
Apply this diff to improve type safety:
-async function retry(fn: any, retries = 3, delay = 5000) {
+async function retry<T>(
+ fn: () => Promise<T | undefined>,
+ retries = 3,
+ delay = 5000
+): Promise<T | undefined> {
for (let i = 0; i < retries; i++) {
try {
const result = await fn();
if (result){
return result;
}
} catch (error) {
console.log(`Screenpipe query failed, retry, attempt: ${i + 1}`)
if (i === retries - 1) throw error;
await new Promise(res => setTimeout(res, delay));
}
}
}
📝 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.
async function retry(fn: any, retries = 3, delay = 5000) { | |
for (let i = 0; i < retries; i++) { | |
try { | |
const result = await fn(); | |
if (result){ | |
return result; | |
} | |
} catch (error) { | |
console.log(`Screenpipe query failed, retry, attempt: ${i + 1}`) | |
if (i === retries - 1) throw error; | |
await new Promise(res => setTimeout(res, delay)); | |
} | |
} | |
} | |
async function retry<T>( | |
fn: () => Promise<T | undefined>, | |
retries = 3, | |
delay = 5000 | |
): Promise<T | undefined> { | |
for (let i = 0; i < retries; i++) { | |
try { | |
const result = await fn(); | |
if (result){ | |
return result; | |
} | |
} catch (error) { | |
console.log(`Screenpipe query failed, retry, attempt: ${i + 1}`) | |
if (i === retries - 1) throw error; | |
await new Promise(res => setTimeout(res, delay)); | |
} | |
} | |
} |
"devDependencies": { | ||
"@tailwindcss/typography": "^0.5.13", | ||
"@types/react-syntax-highlighter": "^15.5.13", | ||
"@types/node": "^20", | ||
"@types/react": "^18", | ||
"@types/react-dom": "^18", | ||
"eslint": "^8", | ||
"eslint-config-next": "14.2.4", | ||
"postcss": "^8", | ||
"tailwindcss": "^3.4.1", | ||
"typescript": "^5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add missing peer dependencies.
The project uses Tailwind CSS but is missing some common peer dependencies.
"devDependencies": {
"@tailwindcss/typography": "^0.5.13",
"@types/react-syntax-highlighter": "^15.5.13",
"@types/node": "^20",
"@types/react": "^18",
"@types/react-dom": "^18",
"eslint": "^8",
"eslint-config-next": "14.2.4",
"postcss": "^8",
"tailwindcss": "^3.4.1",
- "typescript": "^5"
+ "typescript": "^5",
+ "autoprefixer": "^10.0.0",
+ "@types/lodash": "^4.14.0"
📝 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.
"devDependencies": { | |
"@tailwindcss/typography": "^0.5.13", | |
"@types/react-syntax-highlighter": "^15.5.13", | |
"@types/node": "^20", | |
"@types/react": "^18", | |
"@types/react-dom": "^18", | |
"eslint": "^8", | |
"eslint-config-next": "14.2.4", | |
"postcss": "^8", | |
"tailwindcss": "^3.4.1", | |
"typescript": "^5" | |
"devDependencies": { | |
"@tailwindcss/typography": "^0.5.13", | |
"@types/react-syntax-highlighter": "^15.5.13", | |
"@types/node": "^20", | |
"@types/react": "^18", | |
"@types/react-dom": "^18", | |
"eslint": "^8", | |
"eslint-config-next": "14.2.4", | |
"postcss": "^8", | |
"tailwindcss": "^3.4.1", | |
"typescript": "^5", | |
"autoprefixer": "^10.0.0", | |
"@types/lodash": "^4.14.0" | |
} |
"cmdk": "0.2.1", | ||
"lodash": "^4.17.21", | ||
"lucide-react": "^0.468.0", | ||
"next": "14.2.15", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Next.js version 14.2.15 does not exist
The specified version "14.2.15" is invalid and doesn't exist in the npm registry. The latest stable version is 15.1.4, and the latest v14 versions are canary releases of 14.3.0. Please update to a valid Next.js version.
🔗 Analysis chain
Verify Next.js version compatibility.
The specified Next.js version (14.2.15) seems unusual as it's beyond the latest stable version. This might be a typo.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check latest Next.js version
curl -s https://registry.npmjs.org/next/latest | jq -r '.version'
Length of output: 72
Script:
#!/bin/bash
# Check if version 14.2.15 exists and list recent v14 versions
curl -s https://registry.npmjs.org/next | jq -r '.versions | keys[]' | grep "^14\." | sort -V | tail -n 5
Length of output: 192
<Input | ||
id="interval" | ||
name="interval" | ||
type="number" | ||
defaultValue={settings.customSettings?.["reddit-auto-posts"]?.interval || 60} | ||
placeholder="value in seconds" | ||
className="flex-1" | ||
/> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add input validation for interval.
The interval input needs minimum and maximum value constraints to prevent invalid configurations.
<Input
id="interval"
name="interval"
type="number"
+ min="30"
+ max="3600"
defaultValue={settings.customSettings?.["reddit-auto-posts"]?.interval || 60}
placeholder="value in seconds"
className="flex-1"
+ required
/>
📝 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.
<Input | |
id="interval" | |
name="interval" | |
type="number" | |
defaultValue={settings.customSettings?.["reddit-auto-posts"]?.interval || 60} | |
placeholder="value in seconds" | |
className="flex-1" | |
/> | |
</div> | |
<Input | |
id="interval" | |
name="interval" | |
type="number" | |
min="30" | |
max="3600" | |
defaultValue={settings.customSettings?.["reddit-auto-posts"]?.interval || 60} | |
placeholder="value in seconds" | |
className="flex-1" | |
required | |
/> | |
</div> |
const testPipe = async () => { | ||
setLoading(true); | ||
try { | ||
const res = await fetch("/api/pipeline"); | ||
if (res.status === 500 || res.status === 400) { | ||
toast({ | ||
title: "failed to intialize daily log", | ||
description: "please check your credentials", | ||
variant: "destructive" | ||
}) | ||
} else if (res.status === 200) { | ||
toast({ | ||
title: "pipe initalized sucessfully", | ||
variant: "default" | ||
}) | ||
} | ||
const data = await res.json(); | ||
if (data.suggestedQuestions) { | ||
setLastLog(data.suggestedQuestions); | ||
} else { | ||
setLastLog(JSON.stringify(data, null, 2)); | ||
} | ||
} catch (err) { | ||
console.error("error testing log:", err); | ||
} finally { | ||
setLoading(false); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve error handling in testPipe
.
The function has several issues:
- Attempts to parse JSON response even for error cases
- Generic error message for all failure cases
- Missing loading state reset on early returns
const testPipe = async () => {
setLoading(true);
try {
const res = await fetch("/api/pipeline");
+ const data = await res.json();
if (res.status === 500 || res.status === 400) {
+ setLoading(false);
toast({
title: "failed to intialize daily log",
- description: "please check your credentials",
+ description: data.error || "please check your credentials",
variant: "destructive"
})
+ return;
} else if (res.status === 200) {
toast({
title: "pipe initalized sucessfully",
variant: "default"
})
- }
- const data = await res.json();
if (data.suggestedQuestions) {
setLastLog(data.suggestedQuestions);
} else {
setLastLog(JSON.stringify(data, null, 2));
}
+ }
} catch (err) {
console.error("error testing log:", err);
+ toast({
+ title: "Error",
+ description: "Failed to test pipe. Please try again.",
+ variant: "destructive"
+ });
} finally {
setLoading(false);
}
};
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const testPipe = async () => { | |
setLoading(true); | |
try { | |
const res = await fetch("/api/pipeline"); | |
if (res.status === 500 || res.status === 400) { | |
toast({ | |
title: "failed to intialize daily log", | |
description: "please check your credentials", | |
variant: "destructive" | |
}) | |
} else if (res.status === 200) { | |
toast({ | |
title: "pipe initalized sucessfully", | |
variant: "default" | |
}) | |
} | |
const data = await res.json(); | |
if (data.suggestedQuestions) { | |
setLastLog(data.suggestedQuestions); | |
} else { | |
setLastLog(JSON.stringify(data, null, 2)); | |
} | |
} catch (err) { | |
console.error("error testing log:", err); | |
} finally { | |
setLoading(false); | |
} | |
}; | |
const testPipe = async () => { | |
setLoading(true); | |
try { | |
const res = await fetch("/api/pipeline"); | |
const data = await res.json(); | |
if (res.status === 500 || res.status === 400) { | |
setLoading(false); | |
toast({ | |
title: "failed to intialize daily log", | |
description: data.error || "please check your credentials", | |
variant: "destructive" | |
}) | |
return; | |
} else if (res.status === 200) { | |
toast({ | |
title: "pipe initalized sucessfully", | |
variant: "default" | |
}) | |
if (data.suggestedQuestions) { | |
setLastLog(data.suggestedQuestions); | |
} else { | |
setLastLog(JSON.stringify(data, null, 2)); | |
} | |
} | |
} catch (err) { | |
console.error("error testing log:", err); | |
toast({ | |
title: "Error", | |
description: "Failed to test pipe. Please try again.", | |
variant: "destructive" | |
}); | |
} finally { | |
setLoading(false); | |
} | |
}; |
ideally there should be a page to see history of all posts it created https://ui.shadcn.com/docs/components/data-table but could be next step |
btw the questions are quite good i was impressed - even though i use kinda poor model (gemini-1.5-flash-8b) |
how can i get the markdown question at the bottom? if there is no logs yet it can just generate a log and markdown or anything that makes generate the question straight away so the user know this is working and how it looks like |
for this i've to refactor the whole pipeline! |
@louis030195 |
/approve my bad could have gave more instructions in the issue thx for the work! |
@louis030195: The claim has been successfully added to reward-all. You can visit your dashboard to complete the payment. |
still unusable, atm it spams every minute regardless of frequency (which burn lot of AI credits): |
fixing it |
@louis030195 |
i think the email time should be the cron execution time whether its hourly or daily |
/closes #997
/claim #997
the ui is same as the obsidian pipe and make sure to sync the setting across the pipe and app. it'll use the default ai settings as set to the app! i'll add a demo video after doing some posts. its functionality is same as prior
description
brief description of the changes in this pr.
related issue: #
how to test
add a few steps to test the pr in the most time efficient way.
if relevant add screenshots or screen captures to prove that this PR works to save us time.
if you think it can take more than 30 mins for us to review, we will pay between $20 and $200 for someone to review it for us, just ask in the pr.
Summary by CodeRabbit
New Features
User Interface
Technical Improvements
.gitignore
file to maintain a clean repository.