Repository navigation
Upgrade @objectstack to v2.0.0 and migrate Hook API - #128
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…ter hooks Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…ious/result) Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…st files Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
| const isAfterHook = !!ctx.result; | ||
| const isInsert = !ctx.previous; | ||
| const isUpdate = !!ctx.previous; | ||
| const quote = (isBeforeHook ? ctx.input?.doc : ctx.result) as Record<string, any>; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
In general, unused local variables should be removed so that only values that are actually used are computed. This simplifies the code, avoids confusion about intended behavior, and prevents unnecessary computation.
For this specific case, the best fix without changing functionality is to delete the declaration const quote = (isBeforeHook ? ctx.input?.doc : ctx.result) as Record<string, any>; on line 27 inside the handler of QuotePricingHook. No other code in handler uses quote, and the logic already passes ctx to helper functions that derive their own quote. No imports or additional definitions are needed; we are only removing dead code from packages/products/src/hooks/quote.hook.ts.
Concretely:
- Edit
packages/products/src/hooks/quote.hook.ts. - In the
handler: async (ctx: HookContext) => { ... }body, remove the line that declaresconst quote = .... - Leave all other logic intact.
| @@ -24,7 +24,6 @@ | ||
| const isAfterHook = !!ctx.result; | ||
| const isInsert = !ctx.previous; | ||
| const isUpdate = !!ctx.previous; | ||
| const quote = (isBeforeHook ? ctx.input?.doc : ctx.result) as Record<string, any>; | ||
|
|
||
| // Before Insert/Update: Calculate pricing | ||
| if (isBeforeHook) { |
| async function handleQuoteSent(ctx: TriggerContext): Promise<void> { | ||
| const quote = ctx.new; | ||
| async function handleQuoteSent(ctx: any): Promise<void> { | ||
| const quote = ctx.result; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
In general, to fix an unused local variable you either remove the declaration or actually use the variable in a meaningful way. Since handleQuoteSent currently does not use quote at all, the least intrusive and safest fix is to remove the unused declaration const quote = ctx.result;.
Concretely, in packages/products/src/hooks/quote.hook.ts, in the handleQuoteSent function, delete the line declaring quote. No other lines in that function reference quote, so no additional edits are needed. This change does not require new imports, methods, or other definitions.
| @@ -307,7 +307,6 @@ | ||
| * Handle quote sent to customer | ||
| */ | ||
| async function handleQuoteSent(ctx: any): Promise<void> { | ||
| const quote = ctx.result; | ||
|
|
||
| try { | ||
| console.log('📧 Quote sent to customer'); |
| async function handleQuoteExpired(ctx: TriggerContext): Promise<void> { | ||
| const quote = ctx.new; | ||
| async function handleQuoteExpired(ctx: any): Promise<void> { | ||
| const quote = ctx.result; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
In general, to fix an unused-variable finding you either (1) remove the variable (and any dead computation used only to initialize it), or (2) start using the variable meaningfully. To avoid changing existing functionality, the safest approach is to remove the unused declaration if it has no side effects.
Here, handleQuoteExpired declares const quote = ctx.result; and then only logs static messages. Reading ctx.result has no side effects in TypeScript/JavaScript, so we can safely remove that line. No other code in this function or file depends on quote, and we don’t need extra imports or helper methods. Only packages/products/src/hooks/quote.hook.ts needs editing, and only the line const quote = ctx.result; within handleQuoteExpired should be removed.
| @@ -324,7 +324,6 @@ | ||
| * Handle quote expiration | ||
| */ | ||
| async function handleQuoteExpired(ctx: any): Promise<void> { | ||
| const quote = ctx.result; | ||
|
|
||
| try { | ||
| console.log('⏰ Quote expired'); |
| const article = ctx.new; | ||
| const oldArticle = ctx.old; | ||
| const article = ctx.input.doc as Record<string, any>; | ||
| const oldArticle = ctx.previous as Record<string, any> | undefined; |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
In general, unused local variables should be removed if they are not read, unless they are needed for an interface or for side effects (which is not the case here). This eliminates dead code and any confusion about its purpose.
The best fix here is to remove the declaration of oldArticle from the KnowledgeArticleScoringTrigger handler while leaving everything else intact. No other lines depend on oldArticle, and there are no side effects in its initialization (ctx.previous is simply read), so deleting that line does not change runtime behavior. Concretely, edit packages/support/src/hooks/knowledge.hook.ts in the KnowledgeArticleScoringTrigger definition, and delete the line const oldArticle = ctx.previous as Record<string, any> | undefined;. No imports, methods, or additional definitions are needed.
| @@ -18,7 +18,6 @@ | ||
| handler: async (ctx: HookContext) => { | ||
| try { | ||
| const article = ctx.input.doc as Record<string, any>; | ||
| const oldArticle = ctx.previous as Record<string, any> | undefined; | ||
|
|
||
| // Calculate quality score | ||
| article.QualityScore = calculateQualityScore(article); |
There was a problem hiding this comment.
Pull request overview
This PR upgrades the monorepo from @objectstack/* v1.1.0 to v2.0.0 and updates hook implementations/tests/docs to match the v2 Hook API (ctx.input.doc / ctx.result / ctx.previous, plus ctx.ql casting).
Changes:
- Bump
@objectstack/*dependencies across root + workspace packages to^2.0.0(and lockfile updates). - Migrate hook handlers from
ctx.new/ctx.oldtoctx.input.doc/ctx.result/ctx.previousand update related test mocks. - Add
MIGRATION_v2.0.0.mddocumenting the migration patterns.
Reviewed changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Updates lockfile to @objectstack/* v2.0.0 resolution graph. |
| package.json | Bumps root devDependencies to @objectstack/* ^2.0.0. |
| packages/ai/package.json | Bumps @objectstack/spec and peer @objectstack/cli to ^2.0.0. |
| packages/core/package.json | Bumps @objectstack/spec to ^2.0.0. |
| packages/crm/package.json | Bumps @objectstack/spec and peer @objectstack/cli to ^2.0.0. |
| packages/finance/package.json | Bumps @objectstack/spec and peer @objectstack/cli to ^2.0.0. |
| packages/hr/package.json | Bumps @objectstack/spec and peer @objectstack/cli to ^2.0.0. |
| packages/marketing/package.json | Bumps @objectstack/spec and peer @objectstack/cli to ^2.0.0. |
| packages/products/package.json | Bumps @objectstack/spec and peer @objectstack/cli to ^2.0.0. |
| packages/server/package.json | Bumps server runtime dependencies to @objectstack/* 2.0.0. |
| packages/support/package.json | Bumps @objectstack/spec and peer @objectstack/cli to ^2.0.0. |
| packages/support/src/hooks/knowledge.hook.ts | Migrates knowledge hooks to ctx.input.doc / ctx.result / ctx.previous. |
| packages/support/src/hooks/case.hook.ts | Migrates case entitlement hook to ctx.input.doc. |
| packages/products/src/hooks/quote.hook.ts | Migrates quote hook to v2 context access pattern and adds before/after branching logic. |
| packages/products/src/hooks/product.hook.ts | Migrates product hook to v2 context access and adds before/after branching logic. |
| packages/products/src/hooks/pricebook.hook.ts | Migrates pricebook hook to v2 context access and adds before/after branching logic. |
| packages/marketing/src/hooks/roi.hook.ts | Migrates ROI hook to ctx.result / ctx.previous. |
| packages/marketing/src/hooks/campaign_member.hook.ts | Migrates campaign member hooks to v2 context access and loosens ctx typing. |
| packages/marketing/src/hooks/campaign.hook.ts | Migrates campaign hooks and casts ctx.ql to any for ObjectQL usage. |
| packages/hr/src/hooks/performance_review.hook.ts | Migrates performance review hooks and casts ctx.ql to any. |
| packages/hr/src/hooks/offer.hook.ts | Migrates offer hooks and casts ctx.ql to any. |
| packages/hr/src/hooks/employee.hook.ts | Migrates employee hooks and casts ctx.ql to any. |
| packages/hr/src/hooks/candidate.hook.ts | Migrates candidate hooks and casts ctx.ql to any. |
| packages/finance/src/hooks/contract_renewal.hook.ts | Migrates contract renewal hooks to ctx.result / ctx.previous. |
| packages/finance/src/hooks/contract.hook.ts | Migrates contract billing hook to ctx.result / ctx.previous. |
| packages/crm/src/hooks/opportunity.hook.ts | Migrates opportunity hooks to v2 context access. |
| packages/crm/src/hooks/lead.hook.ts | Casts ctx.ql to any for ObjectQL calls. |
| packages/crm/src/hooks/contact.hook.ts | Migrates contact hooks to v2 context access. |
| packages/crm/src/hooks/activity.hook.ts | Migrates activity hooks to v2 context access. |
| packages/crm/src/hooks/account.hook.ts | Migrates account hooks to v2 context access. |
| packages/hr/tests/unit/hooks/offer.hook.test.ts | Updates hook test mocks to include input: { doc }, result, previous. |
| packages/hr/tests/unit/hooks/employee.hook.test.ts | Updates hook test mocks to include input: { doc }, result, previous. |
| packages/hr/tests/unit/hooks/candidate.hook.test.ts | Updates hook test mocks to include input: { doc }, result, previous. |
| packages/finance/tests/unit/hooks/contract_renewal.hook.test.ts | Updates finance hook test contexts to use result / previous. |
| MIGRATION_v2.0.0.md | Adds repo-local migration guide for HookContext changes and testing patterns. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
packages/crm/src/hooks/opportunity.hook.ts:17
- The Opportunity schema’s stage values are "Closed Won"/"Closed Lost" (see
packages/crm/src/opportunity.object.ts), but this validation checks for'closed_won'. This means the validation won’t run when an opportunity is actually moved to Closed Won.
// 1. Validate "closed_won"
if (opp.stage === 'closed_won') {
if (!opp.amount || opp.amount <= 0) {
throw new Error('Validation Error: Cannot close a deal with zero amount. Please update the Amount field.');
}
| // Determine if this is a before or after hook based on available properties | ||
| const isBeforeHook = !!ctx.input; | ||
| const isAfterHook = !!ctx.result; | ||
| const isInsert = !ctx.previous; | ||
| const isUpdate = !!ctx.previous; |
There was a problem hiding this comment.
ctx.input can exist for both before* and after* events in the v2 HookContext, so !!ctx.input is not a reliable way to detect a “before hook”. This can cause the before-logic to run during afterUpdate/afterInsert. Branch on ctx.event (e.g., explicit event comparisons or startsWith('before')/('after')) instead of property presence.
| // Determine if this is a before or after hook based on available properties | |
| const isBeforeHook = !!ctx.input; | |
| const isAfterHook = !!ctx.result; | |
| const isInsert = !ctx.previous; | |
| const isUpdate = !!ctx.previous; | |
| // Determine hook phase and operation from the event name | |
| const isBeforeHook = ctx.event.startsWith('before'); | |
| const isAfterHook = ctx.event.startsWith('after'); | |
| const isInsert = ctx.event.endsWith('Insert'); | |
| const isUpdate = ctx.event.endsWith('Update'); |
| async function calculateQuotePricing(ctx: any): Promise<void> { | ||
| const quote = ctx.input?.doc || ctx.result; | ||
|
|
||
| try { | ||
| // Calculate Discount Amount from Percent or vice versa |
There was a problem hiding this comment.
This hook reads/writes Quote fields using PascalCase keys (e.g., Subtotal, DiscountPercent, TotalPrice), but the Quote schema defines snake_case fields (e.g., subtotal, discount_percent, total_price). As written, pricing/approval calculations won’t update the actual persisted fields. Align field names with packages/products/src/quote.object.ts.
| // Determine if this is a before or after hook based on available properties | ||
| const isBeforeHook = !!ctx.input; | ||
| const isAfterHook = !!ctx.result; |
There was a problem hiding this comment.
ctx.input can exist for after* events too, so !!ctx.input isn’t a safe before/after discriminator. This can cause the before validation logic to run during afterUpdate. Use ctx.event to branch instead.
| // Determine if this is a before or after hook based on available properties | |
| const isBeforeHook = !!ctx.input; | |
| const isAfterHook = !!ctx.result; | |
| // Determine if this is a before or after hook based on the event name | |
| const isBeforeHook = ctx.event === 'beforeInsert' || ctx.event === 'beforeUpdate'; | |
| const isAfterHook = ctx.event === 'afterUpdate'; |
| // Determine if this is a before or after hook based on available properties | ||
| const isBeforeHook = !!ctx.input; | ||
| const isAfterHook = !!ctx.result; |
There was a problem hiding this comment.
ctx.input can exist for after* events too, so !!ctx.input isn’t a safe before/after discriminator. This can cause the before validation logic to run during afterUpdate. Use ctx.event to branch instead.
| // Determine if this is a before or after hook based on available properties | |
| const isBeforeHook = !!ctx.input; | |
| const isAfterHook = !!ctx.result; | |
| // Determine if this is a before or after hook based on the event type | |
| const isBeforeHook = ctx.event === 'beforeInsert' || ctx.event === 'beforeUpdate'; | |
| const isAfterHook = ctx.event === 'afterUpdate'; |
| // Check if Stage actually changed | ||
| const stageChanged = ctx.old.Stage !== ctx.new.Stage; | ||
| const stageChanged = (ctx.previous as Record<string, any> | undefined)?.Stage !== (ctx.result as Record<string, any>)?.Stage; | ||
| if (!stageChanged) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Opportunity schema uses stage (snake_case) and option values like "Closed Won"/"Closed Lost", but this hook compares Stage and values like 'closed_won'/'closed_lost'. That will prevent stage-change detection and downstream automation from running. Update to use the schema’s stage field/value conventions (packages/crm/src/opportunity.object.ts).
| const isAfterHook = !!ctx.result; | ||
| const isInsert = !ctx.previous; | ||
| const isUpdate = !!ctx.previous; | ||
| const quote = (isBeforeHook ? ctx.input?.doc : ctx.result) as Record<string, any>; |
There was a problem hiding this comment.
Unused variable quote.
| const quote = (isBeforeHook ? ctx.input?.doc : ctx.result) as Record<string, any>; |
| async function handleQuoteSent(ctx: TriggerContext): Promise<void> { | ||
| const quote = ctx.new; | ||
| async function handleQuoteSent(ctx: any): Promise<void> { | ||
| const quote = ctx.result; |
There was a problem hiding this comment.
Unused variable quote.
| async function handleQuoteExpired(ctx: TriggerContext): Promise<void> { | ||
| const quote = ctx.new; | ||
| async function handleQuoteExpired(ctx: any): Promise<void> { | ||
| const quote = ctx.result; |
There was a problem hiding this comment.
Unused variable quote.
| const quote = ctx.result; |
| const article = ctx.new; | ||
| const oldArticle = ctx.old; | ||
| const article = ctx.input.doc as Record<string, any>; | ||
| const oldArticle = ctx.previous as Record<string, any> | undefined; |
There was a problem hiding this comment.
Unused variable oldArticle.
Description
Upgraded @objectstack packages from 1.1.0 to 2.0.0. The Hook API has breaking changes in context property access patterns.
Breaking Change:
HookContextrestructuredType of Change
Changes Made
Dependencies (10 files)
@objectstack/{cli,spec,runtime,core,metadata,objectql,plugin-hono-server}→2.0.0Hook Implementations (19 files)
ctx.new→ctx.input.doc(before events) orctx.result(after events)ctx.old→ctx.previous(ctx: HookContext)→(ctx: any)due to Zod type strictnessctx.qlusageTest Mocks (4 files)
{ doc: data }for before eventsresultandpreviousproperties for after eventsDocumentation
MIGRATION_v2.0.0.mdcreated with migration patterns for all event typesTesting
npm test) - 496/496 tests passingnpm run lint)npm run build) - all 9 packages compilepnpm validatepassesChecklist
Additional Notes
CodeQL scan: 0 vulnerabilities detected.
Migration affects all hook handlers across CRM (5), Finance (2), Marketing (3), Products (3), Support (2), and HR (4) packages. Type safety reduced due to
unknowntyping in upstream spec -anycasts required for practical usage.Original prompt
Created from VS Code.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.