-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Add Scheduled and Manual Title Generation for PostGroup #11
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
Conversation
WalkthroughAdded Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Timer as node-cron (every 6h)
participant Mod as postGroup.ts
participant Gen as generateTitle
participant Log as Console
Timer->>Mod: Trigger scheduled task
Mod->>Mod: Iterate postGroups
alt postGroups non-empty
loop for each PostGroup
Mod->>Gen: generateTitle(concatenated post contents)
Gen-->>Mod: Title string
Mod->>Mod: assign group.title
Mod->>Log: Log generated title (context: CRON)
end
else no groups
Mod->>Log: Log "No PostGroup found"
end
Note over Mod,Gen: Errors are caught and logged
sequenceDiagram
autonumber
actor CLI as Node (direct run)
participant Mod as postGroup.ts
participant Gen as generateTitle
participant Log as Console
CLI->>Mod: Execute (require.main === module)
Mod->>Mod: Iterate postGroups
alt postGroups non-empty
loop for each PostGroup
Mod->>Gen: generateTitle(concatenated post contents)
Gen-->>Mod: Title string
Mod->>Mod: assign group.title
Mod->>Log: Log generated title (context: MANUAL)
end
else no groups
Mod->>Log: Log "No PostGroup found"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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: 2
🧹 Nitpick comments (4)
package.json (1)
19-19
: Verify node-cron interop and pin if needed.
- Ensure tsconfig has
esModuleInterop
orallowSyntheticDefaultImports
enabled; otherwiseimport cron from 'node-cron'
in src/postGroup.ts will fail at compile time.- Consider pinning to an exact version to avoid scheduler behavior drifting on prod.
Add a convenience script for manual runs:
"scripts": { "classify": "tsx run-classifier.ts", + "postgroup:title": "tsx src/postGroup.ts", "classify:complete": "tsx run-classifier.ts complete",
src/postGroup.ts (3)
1-2
: Confirm default import shape for node-cron.
import cron from 'node-cron'
assumes default export + TS interop. If your tsconfig lacksesModuleInterop
/allowSyntheticDefaultImports
, switch to named import (import { schedule } from 'node-cron'
) and callschedule(...)
.
22-26
: Guard empty groups, cap payload size, and avoid logging raw content on failures.
- Empty
posts
yields an empty prompt to OpenAI.- Large groups can blow token limits.
- The referenced generateTitleForPost logs the full input on errors; passing the aggregated content risks leaking lots of text to logs.
Apply:
export async function generateTitleForPostGroup(postGroup: PostGroup): Promise<string> { - const combinedContent = postGroup.posts.map(post => post.content).join('\n\n'); - return await generateTitleForPost(combinedContent); + if (!postGroup?.posts?.length) { + throw new Error(`PostGroup ${postGroup?.id ?? 'unknown'} has no posts`); + } + const MAX_CHARS = 3500; + let combinedContent = postGroup.posts.map(p => p.content.trim()).join('\n\n'); + if (combinedContent.length > MAX_CHARS) { + combinedContent = combinedContent.slice(0, MAX_CHARS) + '…'; + } + try { + return await generateTitleForPost(combinedContent); + } catch (e) { + // Avoid logging raw combined content; include only metadata + throw new Error(`Title generation failed for PostGroup ${postGroup.id} (posts=${postGroup.posts.length}): ${String(e)}`); + } }
28-101
: Seed data nits: droplink: undefined
and consider fixed timestamps.
- Explicit
link: undefined
is noisy; omit the key.- If this data is ever snapshot-tested,
new Date().toISOString()
at import time creates nondeterminism.- link: undefined, + // link intentionally omitted - createdAt: new Date().toISOString(), - updatedAt: new Date().toISOString(), + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(),If determinism is desired, replace with fixed ISO strings in fixtures.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json
(1 hunks)src/postGroup.ts
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/postGroup.ts (1)
src/generateTitle.ts (1)
generateTitleForPost
(18-52)
src/postGroup.ts
Outdated
// Schedule a cron job to generate and log the title for the first PostGroup every 6 hours | ||
cron.schedule('0 */6 * * *', async () => { | ||
const group = postGroups[0]; | ||
if (!group) { | ||
console.log('No PostGroup found.'); | ||
return; | ||
} | ||
try { | ||
const title = await generateTitleForPostGroup(group); | ||
console.log(`[CRON] Generated Title for PostGroup at ${new Date().toISOString()}:`, title); | ||
} catch (e) { | ||
console.error('[CRON] Error generating title:', e); | ||
} | ||
}); |
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
Avoid side effects on import; add timezone and idempotency.
Scheduling at module top-level starts a job whenever the module is imported (tests, scripts, REPL). Hot-reload can create duplicate jobs. Also, no timezone is specified.
Apply:
-// Schedule a cron job to generate and log the title for the first PostGroup every 6 hours
-cron.schedule('0 */6 * * *', async () => {
- const group = postGroups[0];
- if (!group) {
- console.log('No PostGroup found.');
- return;
- }
- try {
- const title = await generateTitleForPostGroup(group);
- console.log(`[CRON] Generated Title for PostGroup at ${new Date().toISOString()}:`, title);
- } catch (e) {
- console.error('[CRON] Error generating title:', e);
- }
-});
+// Startable cron to generate and log the title for the first PostGroup every 6 hours
+export function startPostGroupTitleCron(tz = process.env.TZ || 'UTC') {
+ return cron.schedule('0 */6 * * *', async () => {
+ const group = postGroups[0];
+ if (!group) {
+ console.log('[CRON] No PostGroup found.');
+ return;
+ }
+ try {
+ const title = await generateTitleForPostGroup(group);
+ group.title = title; // persist locally if desired
+ console.log(`[CRON] Generated Title @ ${new Date().toISOString()}:`, title);
+ } catch (e) {
+ console.error('[CRON] Error generating title:', e);
+ }
+ }, { timezone: tz });
+}
Usage: call startPostGroupTitleCron()
from your app bootstrap or behind a feature flag/env.
📝 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.
// Schedule a cron job to generate and log the title for the first PostGroup every 6 hours | |
cron.schedule('0 */6 * * *', async () => { | |
const group = postGroups[0]; | |
if (!group) { | |
console.log('No PostGroup found.'); | |
return; | |
} | |
try { | |
const title = await generateTitleForPostGroup(group); | |
console.log(`[CRON] Generated Title for PostGroup at ${new Date().toISOString()}:`, title); | |
} catch (e) { | |
console.error('[CRON] Error generating title:', e); | |
} | |
}); | |
// Startable cron to generate and log the title for the first PostGroup every 6 hours | |
export function startPostGroupTitleCron(tz = process.env.TZ || 'UTC') { | |
return cron.schedule('0 */6 * * *', async () => { | |
const group = postGroups[0]; | |
if (!group) { | |
console.log('[CRON] No PostGroup found.'); | |
return; | |
} | |
try { | |
const title = await generateTitleForPostGroup(group); | |
group.title = title; // persist locally if desired | |
console.log(`[CRON] Generated Title @ ${new Date().toISOString()}:`, title); | |
} catch (e) { | |
console.error('[CRON] Error generating title:', e); | |
} | |
}, { timezone: tz }); | |
} |
🤖 Prompt for AI Agents
In src/postGroup.ts around lines 106 to 119, the cron is scheduled at module
top-level causing side effects on import, no timezone is set, and repeated
imports/hot-reloads can register duplicate jobs; refactor by moving the cron
setup into an exported startPostGroupTitleCron() function that (1) returns or
stores a reference to the scheduled job so it can be canceled, (2) guards
against multiple starts by checking/clearing an existing job before scheduling
(idempotency), and (3) passes a timezone option (e.g., from config or
process.env) to cron.schedule; call startPostGroupTitleCron() from your app
bootstrap or behind a feature-flag/env rather than importing the module to
schedule automatically.
src/postGroup.ts
Outdated
// If this file is run directly, generate and log the title for the first PostGroup | ||
if (require.main === module) { | ||
(async () => { | ||
const group = postGroups[0]; | ||
if (!group) { | ||
console.log('No PostGroup found.'); | ||
return; | ||
} | ||
try { | ||
const title = await generateTitleForPostGroup(group); | ||
console.log('Title:', title); | ||
} catch (e) { | ||
console.error('Error generating title:', e); | ||
} | ||
})(); | ||
} No newline at end of file |
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.
Main-guard: make it safe and explicit, and start the cron only here.
As written, the block assumes require
exists. Make it resilient and ensure the cron starts only on direct execution.
-// If this file is run directly, generate and log the title for the first PostGroup
-if (require.main === module) {
+// If this file is run directly, generate and log the title for the first PostGroup
+const isDirectRun = typeof require !== 'undefined' && require.main === module;
+if (isDirectRun) {
+ // optional: start the cron when run directly
+ const task = startPostGroupTitleCron();
(async () => {
const group = postGroups[0];
if (!group) {
console.log('No PostGroup found.');
return;
}
try {
const title = await generateTitleForPostGroup(group);
- console.log('Title:', title);
+ group.title = title;
+ console.log('[MANUAL] Title:', title);
} catch (e) {
console.error('Error generating title:', e);
}
})();
+ // keep process alive if cron is running
+ task.start();
}
If you later switch the repo to ESM ("type": "module"
), replace the guard with an import.meta.url
check.
🤖 Prompt for AI Agents
In src/postGroup.ts around lines 121 to 136, the current main-guard assumes
`require` exists and may break under ESM; make the guard resilient by checking
`typeof require !== 'undefined' && require.main === module` before running the
IIFE, and move/ensure the cron/job startup code is only invoked from inside this
same guarded block so the cron starts only on direct execution; also add a
comment noting that if the repo becomes ESM the guard should be replaced with an
`import.meta.url` check.
…hts and remove links
…improve error handling
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: 0
♻️ Duplicate comments (2)
src/postGroup.ts (2)
100-102
: Avoid side effects on import; add timezone and idempotency.Scheduling at module top-level starts a job whenever the module is imported (tests, scripts, REPL). Hot-reload can create duplicate jobs. Also, no timezone is specified.
Apply:
-// Schedule a cron job to generate and log the title for all PostGroups every 6 hours -cron.schedule('0 */6 * * *', async () => { - await logTitlesForAllPostGroups('CRON'); -}); +// Startable cron to generate and log the title for all PostGroups every 6 hours +export function startPostGroupTitleCron(tz = process.env.TZ || 'UTC') { + return cron.schedule('0 */6 * * *', async () => { + await logTitlesForAllPostGroups('CRON'); + }, { + timezone: tz, + scheduled: false // Don't start automatically + }); +}Usage: call
startPostGroupTitleCron().start()
from your app bootstrap or behind a feature flag/env.
105-107
: Main-guard: make it safe and explicit, and start the cron only here.As written, the block assumes
require
exists. Make it resilient and ensure the cron starts only on direct execution.-// If this file is run directly, generate and log the title for all PostGroups -if (require.main === module) { - logTitlesForAllPostGroups('MANUAL'); -} +// If this file is run directly, generate and log the title for all PostGroups +const isDirectRun = typeof require !== 'undefined' && require.main === module; +if (isDirectRun) { + // Start the cron when run directly and run manual execution + const task = startPostGroupTitleCron(); + task.start(); + logTitlesForAllPostGroups('MANUAL'); +}If you later switch the repo to ESM (
"type": "module"
), replace the guard with animport.meta.url
check.
🧹 Nitpick comments (1)
src/postGroup.ts (1)
29-68
: Consider extracting sample data to a separate file.The hardcoded sample data makes the module less reusable and mixes concerns. Consider moving this to a dedicated test data file or configuration.
// Create src/data/samplePostGroups.ts export const samplePostGroups: PostGroup[] = [ // ... current sample data ];Then import it when needed:
-const postGroups: PostGroup[] = [ - // ... sample data -]; +import { samplePostGroups } from './data/samplePostGroups'; + +const postGroups: PostGroup[] = samplePostGroups;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/postGroup.ts
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/postGroup.ts (1)
src/generateTitle.ts (1)
generateTitleForPost
(18-52)
🔇 Additional comments (4)
src/postGroup.ts (4)
1-2
: LGTM!Clean imports with appropriate dependencies for cron scheduling and title generation functionality.
4-20
: Well-structured type definitions.The
Post
andPostGroup
types are comprehensive and properly typed with appropriate union types for sentiment and source enums.
23-26
: LGTM!The function correctly aggregates post content and delegates to the existing title generation utility. The implementation is clean and follows the single responsibility principle.
75-97
: LGTM!Good implementation with proper error handling, context-aware logging, and mutation of the group's title property for persistence.
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.
this is good but we need another pr for upstash integration.
Description:
npx tsx src/postGroup.ts
) will generate and print the title immediately for testing.Summary by CodeRabbit
New Features
Chores