fix: ensure owner is present throughout the jobs#1816
Conversation
WalkthroughThis update removes the monolithic Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant JobRepository
participant FindOrCreateJob
participant CheckInProgressJob
participant CheckJobLimit
participant CancelJob
participant StartJob
participant CreateWorkspace
participant CreateFlashcards
participant BuildDeck
participant NotifyUser
participant SetJobFailed
participant CompleteJob
Controller->>FindOrCreateJob: execute({id, owner, title, type})
FindOrCreateJob->>JobRepository: findJobById(id, owner)
alt Job exists
FindOrCreateJob-->>Controller: return job
else
FindOrCreateJob->>JobRepository: create(id, owner, title, type)
FindOrCreateJob->>JobRepository: findJobById(id, owner)
FindOrCreateJob-->>Controller: return job
end
Controller->>CheckInProgressJob: execute(id, owner)
alt In progress or failed
Controller-->>Client: return (job already in progress/failed)
else
Controller->>CheckJobLimit: execute({owner, maxJobs})
alt Limit exceeded
Controller->>CancelJob: execute({id, owner, reason})
Controller-->>Client: return (job cancelled)
else
Controller->>StartJob: execute({id, owner})
Controller->>CreateWorkspace: execute({id, owner, api, jobRepository})
Controller->>CreateFlashcards: execute({bl, id, owner, rules, settings, type})
Controller->>BuildDeck: execute({bl, exporter, decks, ws, settings, storage, id, owner})
Controller->>NotifyUser: execute({owner, rules, db, key, id, size, apkg})
Controller->>CompleteJob: execute(id, owner)
Controller-->>Client: return (job started)
end
end
Note over Controller,Client: On error, SetJobFailed is called and error returned to client if waiting
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error code ERESOLVE ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
aalemayhu
force-pushed
the
fix/missing-owner-column
branch
from
d8dd471 to
f4a5da7
Compare
Fixes this report: [ERROR] [2anki.net] - Undefined binding(s) detected when compiling SELECT. Undefined column(s): [owner] query: select * from "jobs" where "owner" = ? Signed-off-by: Alexander Alemayhu <alexander@alemayhu.com>
aalemayhu
force-pushed
the
fix/missing-owner-column
branch
from
f4a5da7 to
f2143d3
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
src/usecases/jobs/CreateJobUseCase.tsx (1)
1-22: Fix file extension and clarify return type assumption.
- The file extension should be
.tsinstead of.tsxsince this contains TypeScript without JSX.- The method assumes
jobRepository.create()returns an array and takes the first element as a number, but this assumption should be verified or documented.-// File: src/usecases/jobs/CreateJobUseCase.tsx +// File: src/usecases/jobs/CreateJobUseCase.tsConsider adding a comment explaining what
result[0]represents or ensure the repository method's return type contract is well-defined.src/usecases/jobs/CreateFlashcardsForJobUseCase.ts (1)
32-38: Add explanation for ID dash removal.The code removes dashes from the ID with
id.replace(/-/g, '')but there's no comment explaining why this transformation is necessary.return bl.findFlashcards({ parentType: type ?? 'page', + // Remove dashes to match expected ID format for findFlashcards topLevelId: id.replace(/-/g, ''), rules, decks: [], parentName: settings.deckName || '', });src/usecases/jobs/BuildDeckForJobUseCase.ts (1)
50-50: Remove unnecessary type assertion.The type assertion
as stringappears unnecessary and could mask type issues. Ifgen.run()doesn't return the expected type, it's better to fix the return type or handle it explicitly.- const payload = (await gen.run()) as string; + const payload = await gen.run();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/controllers/NotionController.ts(1 hunks)src/data_layer/JobRepository.ts(2 hunks)src/lib/storage/jobs/ConversionJob.ts(0 hunks)src/lib/storage/jobs/helpers/notifyUserIfNecessary.ts(0 hunks)src/lib/storage/jobs/helpers/performConversion.ts(3 hunks)src/services/EmailService/templates/convert-link.html(2 hunks)src/services/NotionService/BlockHandler/BlockHandler.ts(2 hunks)src/services/NotionService/blocks/media/BlockBookmark/hooks/useMetadata.ts(1 hunks)src/services/NotionService/helpers/preserveNewlinesIfApplicable.ts(1 hunks)src/usecases/jobs/BuildDeckForJobUseCase.ts(1 hunks)src/usecases/jobs/CancelJobUseCase.ts(1 hunks)src/usecases/jobs/CheckInProgressJobUseCase.ts(1 hunks)src/usecases/jobs/CheckJobLimitUseCase.ts(1 hunks)src/usecases/jobs/CompleteJobUseCase.ts(1 hunks)src/usecases/jobs/CreateFlashcardsForJobUseCase.ts(1 hunks)src/usecases/jobs/CreateJobUseCase.tsx(1 hunks)src/usecases/jobs/CreateJobWorkSpaceUseCase.ts(1 hunks)src/usecases/jobs/FindOrCreateJobUseCase.ts(1 hunks)src/usecases/jobs/NotifyUserUseCase.ts(1 hunks)src/usecases/jobs/SetJobFailedUseCase.ts(1 hunks)src/usecases/jobs/StartJobUseCase.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/lib/storage/jobs/helpers/notifyUserIfNecessary.ts
- src/lib/storage/jobs/ConversionJob.ts
🧰 Additional context used
🧬 Code Graph Analysis (7)
src/usecases/jobs/NotifyUserUseCase.ts (2)
src/data_layer/public/ParserRules.ts (1)
ParserRules(8-24)src/services/EmailService/EmailService.ts (1)
useDefaultEmailService(405-410)
src/usecases/jobs/CancelJobUseCase.ts (1)
src/data_layer/public/Jobs.ts (1)
Jobs(8-26)
src/lib/storage/jobs/helpers/performConversion.ts (12)
src/usecases/jobs/FindOrCreateJobUseCase.ts (1)
FindOrCreateJobUseCase(12-38)src/usecases/jobs/CheckInProgressJobUseCase.ts (1)
CheckInProgressJobUseCase(3-18)src/usecases/jobs/CheckJobLimitUseCase.ts (1)
CheckJobLimitUseCase(8-18)src/lib/isPaying.ts (1)
isPaying(1-6)src/usecases/jobs/CancelJobUseCase.ts (1)
CancelJobUseCase(10-27)src/usecases/jobs/StartJobUseCase.ts (1)
StartJobUseCase(9-26)src/usecases/jobs/CreateJobWorkSpaceUseCase.ts (1)
CreateJobWorkSpaceUseCase(26-60)src/usecases/jobs/CreateFlashcardsForJobUseCase.ts (1)
CreateFlashcardsForJobUseCase(16-40)src/usecases/jobs/SetJobFailedUseCase.ts (1)
SetJobFailedUseCase(3-19)src/usecases/jobs/BuildDeckForJobUseCase.ts (1)
BuildDeckForJobUseCase(36-74)src/usecases/jobs/NotifyUserUseCase.ts (1)
NotifyUserUseCase(17-38)src/usecases/jobs/CompleteJobUseCase.ts (1)
CompleteJobUseCase(3-19)
src/usecases/jobs/StartJobUseCase.ts (1)
src/data_layer/public/Jobs.ts (1)
Jobs(8-26)
src/usecases/jobs/FindOrCreateJobUseCase.ts (2)
src/data_layer/public/Jobs.ts (1)
Jobs(8-26)src/usecases/jobs/CreateJobUseCase.tsx (1)
CreateJobUseCase(10-21)
src/usecases/jobs/BuildDeckForJobUseCase.ts (3)
src/lib/parser/Deck.ts (1)
Deck(4-53)src/lib/anki/format.ts (3)
isValidDeckName(4-5)addDeckNameSuffix(7-8)DECK_NAME_SUFFIX(1-1)src/lib/misc/file.ts (1)
FileSizeInMegaBytes(23-26)
src/data_layer/JobRepository.ts (1)
src/data_layer/public/Jobs.ts (1)
Jobs(8-26)
🔇 Additional comments (17)
src/services/EmailService/templates/convert-link.html (1)
28-30: LGTM! Professional branding improvement.The signature change from individual developer to team branding is a good improvement for professional presentation.
src/controllers/NotionController.ts (1)
94-94: LGTM! Good defensive programming practice.The null coalescing operator ensures
titleis always a string, preventing potential downstream errors when the title is null or undefined.src/services/NotionService/BlockHandler/BlockHandler.ts (2)
200-200: LGTM! Consistent fallback handling.Using
||operator provides a consistent fallback to empty string for falsy values.
218-223: LGTM! Safer conditional processing.Adding the conditional check before calling
preserveNewlinesIfApplicableprevents potential errors whenankiNote.backis falsy and improves robustness.src/usecases/jobs/CheckInProgressJobUseCase.ts (1)
6-17: Owner parameter correctly included, but review the "in progress" logic.The use case correctly includes the
ownerparameter in the job query (findJobById(id, owner)), which directly addresses the PR objective of ensuring owner is present throughout job operations.However, the logic for determining "in progress" status seems questionable:
- Line 12-14: Returns
truefor 'started' status ✓- Line 16: Returns
truefor 'failed' status ❓Why would a 'failed' job be considered "in progress"? This seems counterintuitive - failed jobs are typically terminal states, not in-progress states.
Consider clarifying the intended behavior or renaming the method to better reflect what it actually checks.
src/services/NotionService/helpers/preserveNewlinesIfApplicable.ts (1)
4-9: Good null safety improvement!The updated function signature and guard clause properly handle nullable inputs, preventing potential runtime errors when text is null or undefined.
src/usecases/jobs/CheckJobLimitUseCase.ts (1)
8-18: Clean implementation with proper owner usage!The use case correctly uses the owner parameter in repository calls, aligning with the PR objective of ensuring owner is present throughout job operations.
src/usecases/jobs/SetJobFailedUseCase.ts (1)
6-18: Solid implementation with proper error handling!The use case correctly uses both id and owner parameters for job retrieval, includes appropriate error handling, and has an optimization for already failed jobs.
src/usecases/jobs/CompleteJobUseCase.ts (1)
7-17: Good owner parameter usage!The use case correctly uses both jobId and owner parameters for job retrieval, aligning with the PR objective of ensuring owner is present throughout job operations.
src/usecases/jobs/CancelJobUseCase.ts (1)
10-26: Excellent implementation with consistent owner usage!The use case properly uses both id and owner parameters for job operations, includes appropriate error handling, and has optimization for already cancelled jobs. This aligns perfectly with the PR objective of ensuring owner is present throughout job operations.
src/usecases/jobs/StartJobUseCase.ts (1)
4-26: LGTM! Proper owner field usage throughout.The implementation correctly uses the
ownerfield in bothfindJobByIdandupdateJobStatuscalls, which directly addresses the PR objective of ensuring owner is present throughout job operations. The error handling and cancelled job logic are also appropriate.src/usecases/jobs/NotifyUserUseCase.ts (1)
2-2: No import path mismatch – ParserRules class is correctly importedThe code in NotifyUserUseCase.ts imports the ParserRules class from
src/lib/parser/ParserRules, which is exactly where the class is defined. The interface insrc/data_layer/public/ParserRules.tsisn’t used at that import site, so the concerns about differing paths and property‐name casing do not apply here. Please disregard this comment.Likely an incorrect or invalid review comment.
src/usecases/jobs/CreateFlashcardsForJobUseCase.ts (1)
19-40: LGTM! Proper owner field usage.The use case correctly uses the
ownerfield in theupdateJobStatuscall, which aligns with the PR objective of ensuring owner is present throughout job operations.src/usecases/jobs/BuildDeckForJobUseCase.ts (1)
36-74: Implementation correctly ensures owner presence throughout job operations.The use case properly includes the
ownerparameter in storage operations (line 62) and database insertions (line 67), which aligns with the PR objective of fixing the "owner is undefined" error.src/data_layer/JobRepository.ts (1)
13-53: Repository methods correctly enforce owner presence in all queries.All the repository methods properly include the
ownerparameter in their database queries:
deleteJob(lines 15-16)create(line 25)findJobById(line 33)updateJobStatus(line 45)This ensures that job operations are properly scoped to the owner and prevents the "owner is undefined" error mentioned in the PR objectives.
src/usecases/jobs/CreateJobWorkSpaceUseCase.ts (1)
26-60: Use case correctly includes owner in job operations.The implementation properly passes the
ownerparameter when updating job status (line 34) and loading settings (line 50), ensuring owner presence throughout the job workspace creation process.src/lib/storage/jobs/helpers/performConversion.ts (1)
29-147: Refactoring successfully ensures owner presence throughout job operations.The conversion to a use-case architecture properly addresses the PR objective. All job operations now consistently include the
ownerparameter:
- Job creation (line 45)
- Status checks (line 51)
- Job limits (line 60)
- Job cancellation (line 66)
- All subsequent operations through completion
This architectural change prevents the "owner is undefined" database query error mentioned in the PR objectives.
| require('metascraper-description'), | ||
| require('metascraper-image'), | ||
| require('metascraper-logo-favicon'), | ||
| require('metascraper-title'), | ||
| require('metascraper-url'), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify metascraper API compatibility.
The change from requiring and immediately invoking metascraper modules to just requiring them suggests an API change. Ensure this aligns with the metascraper usage pattern and doesn't break functionality.
🏁 Script executed:
#!/bin/bash
# Description: Check metascraper usage in the codebase to ensure compatibility
# Expected: Find where metascraper is used and verify the pattern is consistent
# Search for metascraper usage patterns
rg -A 3 -B 3 "metascraper\("
# Check if there are any other require patterns for metascraper
rg "require.*metascraper"Length of output: 1489
🌐 Web query:
Metascraper plugin modules invocation: do we call require('metascraper-description')() or just require('metascraper-description') in latest version?
💡 Result:
In the latest version of metascraper-description, you should invoke the module as a function by appending () after requiring it. This pattern is consistent across all metascraper-* modules. Here's how you can use it:
const metascraper = require('metascraper')([
require('metascraper-description')(),
// ... other rules
]);This approach is detailed in the metascraper README. (github.com)
By invoking require('metascraper-description')() in this manner, you ensure that the module is properly initialized and ready for use within your metascraper setup.
Citations:
Re-add invocation parentheses for metascraper plugins
The latest metascraper-* modules export a factory function, so each rule must be invoked with (). Without the invocation, you’re passing the factory itself rather than the initialized rule, which will break at runtime.
Files to update:
- src/services/NotionService/blocks/media/BlockBookmark/hooks/useMetadata.ts (lines 4–8)
Required diff:
- require('metascraper-description'),
- require('metascraper-image'),
- require('metascraper-logo-favicon'),
- require('metascraper-title'),
- require('metascraper-url'),
+ require('metascraper-description')(),
+ require('metascraper-image')(),
+ require('metascraper-logo-favicon')(),
+ require('metascraper-title')(),
+ require('metascraper-url')(),This change aligns with the metascraper README usage pattern.
📝 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.
| require('metascraper-description'), | |
| require('metascraper-image'), | |
| require('metascraper-logo-favicon'), | |
| require('metascraper-title'), | |
| require('metascraper-url'), | |
| require('metascraper-description')(), | |
| require('metascraper-image')(), | |
| require('metascraper-logo-favicon')(), | |
| require('metascraper-title')(), | |
| require('metascraper-url')(), |
🤖 Prompt for AI Agents
In src/services/NotionService/blocks/media/BlockBookmark/hooks/useMetadata.ts
between lines 4 and 8, the metascraper plugins are imported without invoking
their factory functions. To fix this, add parentheses after each require
statement to call the factory function, e.g., change
require('metascraper-description') to require('metascraper-description')(). This
ensures each plugin is properly initialized as required by the latest
metascraper versions.
| export class CompleteJobUseCase { | ||
| constructor(private readonly jobRepository: JobRepository) {} | ||
|
|
||
| async execute(jobId: string, owner: string): Promise<number> { |
There was a problem hiding this comment.
Fix return type mismatch!
The method signature declares Promise<number> but the implementation returns different types.
Apply this diff to fix the type consistency:
- async execute(jobId: string, owner: string): Promise<number> {
+ async execute(jobId: string, owner: string): Promise<Jobs | number> {Or if you intend to always return a number, update line 14:
- return job;
+ return 0; // or appropriate number indicating cancelled job📝 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 execute(jobId: string, owner: string): Promise<number> { | |
| async execute(jobId: string, owner: string): Promise<Jobs | number> { |
🤖 Prompt for AI Agents
In src/usecases/jobs/CompleteJobUseCase.ts at line 6, the execute method's
return type is declared as Promise<number> but the implementation returns
different types. To fix this, ensure that all return statements inside the
method return a number wrapped in a Promise. If the method should always return
a number, update the return statements accordingly to maintain type consistency
with the declared Promise<number> return type.
| interface FindOrCreateJobUseCaseInput { | ||
| id: string; | ||
| owner: string; | ||
| title: string; | ||
| type: string; | ||
| } |
There was a problem hiding this comment.
Interface inconsistency with CreateJobUseCase.
The title and type fields are required in FindOrCreateJobUseCaseInput but optional in CreateJobUseCaseIn. This inconsistency could lead to runtime issues.
interface FindOrCreateJobUseCaseInput {
id: string;
owner: string;
- title: string;
- type: string;
+ title?: string | null;
+ type?: string;
}📝 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.
| interface FindOrCreateJobUseCaseInput { | |
| id: string; | |
| owner: string; | |
| title: string; | |
| type: string; | |
| } | |
| interface FindOrCreateJobUseCaseInput { | |
| id: string; | |
| owner: string; | |
| title?: string | null; | |
| type?: string; | |
| } |
🤖 Prompt for AI Agents
In src/usecases/jobs/FindOrCreateJobUseCase.ts between lines 5 and 10, the
interface FindOrCreateJobUseCaseInput declares the fields title and type as
required, while in CreateJobUseCaseInput these fields are optional. To fix this
inconsistency and avoid runtime issues, update the FindOrCreateJobUseCaseInput
interface to make the title and type fields optional by adding a question mark
(?) after their names.
| const createJob = new CreateJobUseCase(this.jobRepository); | ||
| await createJob.execute({ | ||
| id, | ||
| owner, | ||
| title, | ||
| type, | ||
| }); | ||
|
|
||
| const secondLookup = await this.jobRepository.findJobById(id, owner); | ||
| if (!secondLookup) { | ||
| throw new Error('Failed to find or create job after creation attempt'); | ||
| } | ||
| return secondLookup; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring dependency injection and second lookup.
-
Dependency Injection: The
CreateJobUseCaseis instantiated inside the method, which makes testing difficult and violates dependency inversion. -
Unnecessary Second Lookup: The second lookup after job creation seems redundant if the create operation could return the created job entity.
export class FindOrCreateJobUseCase {
- constructor(private readonly jobRepository: JobRepository) {}
+ constructor(
+ private readonly jobRepository: JobRepository,
+ private readonly createJobUseCase: CreateJobUseCase
+ ) {}
async execute(input: FindOrCreateJobUseCaseInput): Promise<Jobs> {
const { id, owner, title, type } = input;
const existingJob = await this.jobRepository.findJobById(id, owner);
if (existingJob) {
return existingJob;
}
- const createJob = new CreateJobUseCase(this.jobRepository);
- await createJob.execute({
+ const createdJobId = await this.createJobUseCase.execute({
id,
owner,
title,
type,
});
- const secondLookup = await this.jobRepository.findJobById(id, owner);
- if (!secondLookup) {
- throw new Error('Failed to find or create job after creation attempt');
- }
- return secondLookup;
+ // If create returns the job entity instead of ID, this could be simplified
+ return await this.jobRepository.findJobById(id, owner);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/usecases/jobs/FindOrCreateJobUseCase.ts around lines 24 to 36, refactor
the code to inject CreateJobUseCase via the constructor or as a class dependency
instead of instantiating it inside the method to improve testability and adhere
to dependency inversion. Also, modify the createJob.execute method to return the
created job entity directly, eliminating the need for the redundant second
lookup from the repository after creation.
| async execute(input: NotifyUserUseCaseInput): Promise<void> { | ||
| const { owner, rules, db, key, id, size, apkg } = input; | ||
|
|
||
| console.debug('rules.email', rules.EMAIL_NOTIFICATION); |
There was a problem hiding this comment.
Property access mismatch with interface definition.
The code accesses rules.EMAIL_NOTIFICATION but the ParserRules interface defines the property as email_notification (lowercase). This will cause a runtime error.
-console.debug('rules.email', rules.EMAIL_NOTIFICATION);
+console.debug('rules.email', rules.email_notification);📝 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.
| console.debug('rules.email', rules.EMAIL_NOTIFICATION); | |
| console.debug('rules.email', rules.email_notification); |
🤖 Prompt for AI Agents
In src/usecases/jobs/NotifyUserUseCase.ts at line 23, the code accesses the
property rules.EMAIL_NOTIFICATION, but the ParserRules interface defines this
property as email_notification in lowercase. To fix this, change the property
access to rules.email_notification to match the interface definition and avoid
runtime errors.
| const updateJobStatus = await this.jobRepository.updateJobStatus( | ||
| id, | ||
| owner, | ||
| 'step2_creating_flashcards', | ||
| '' | ||
| ); | ||
|
|
||
| if (!updateJobStatus) { | ||
| throw new Error('Failed to update job status'); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify return type assumption for status update check.
The code checks if (!updateJobStatus) but it's unclear what the updateJobStatus method returns. This assumption should be verified or documented.
🏁 Script executed:
#!/bin/bash
# Check JobRepository.updateJobStatus method signature and return type
ast-grep --pattern 'updateJobStatus($$$) { $$$ }'
rg -A 5 "updateJobStatus.*:" src/data_layer/JobRepository.tsLength of output: 114
🏁 Script executed:
#!/bin/bash
set -e
# Search for all occurrences and definitions of updateJobStatus in TypeScript files
rg -n "updateJobStatus" -t tsLength of output: 779
🏁 Script executed:
#!/bin/bash
set -e
# Show the signature and implementation of updateJobStatus in JobRepository.ts
rg -n -A10 -B2 "async updateJobStatus" src/data_layer/JobRepository.tsLength of output: 416
#!/bin/bash
set -e
# Display full implementation of updateJobStatus to confirm its return behavior
rg -n -A20 -B2 "async updateJobStatus" src/data_layer/JobRepository.ts🏁 Script executed:
#!/bin/bash
set -e
# Show extended implementation of updateJobStatus to verify its return behavior
rg -n -A50 -B0 "async updateJobStatus" src/data_layer/JobRepository.tsLength of output: 564
Clarify updateJobStatus return type and error handling
The updateJobStatus method actually returns a Jobs object (or undefined if no row was updated), not a boolean. To make this clear and avoid confusion, rename the variable and use an explicit null/undefined check:
• File: src/usecases/jobs/CreateFlashcardsForJobUseCase.ts
Lines: ~21–30
Suggested diff:
- const updateJobStatus = await this.jobRepository.updateJobStatus(
+ const updatedJob = await this.jobRepository.updateJobStatus(
id,
owner,
'step2_creating_flashcards',
''
- );
-
- if (!updateJobStatus) {
- throw new Error('Failed to update job status');
- }
+ );
+
+ if (updatedJob == null) {
+ throw new Error(`Failed to update job status for job ${id}`);
+ }This makes it clear you’re working with the returned job record and explicitly handling the “no row updated” case.
📝 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 updateJobStatus = await this.jobRepository.updateJobStatus( | |
| id, | |
| owner, | |
| 'step2_creating_flashcards', | |
| '' | |
| ); | |
| if (!updateJobStatus) { | |
| throw new Error('Failed to update job status'); | |
| } | |
| const updatedJob = await this.jobRepository.updateJobStatus( | |
| id, | |
| owner, | |
| 'step2_creating_flashcards', | |
| '' | |
| ); | |
| if (updatedJob == null) { | |
| throw new Error(`Failed to update job status for job ${id}`); | |
| } |
🤖 Prompt for AI Agents
In src/usecases/jobs/CreateFlashcardsForJobUseCase.ts around lines 21 to 30, the
variable updateJobStatus is misleading because the method returns a Jobs object
or undefined, not a boolean. Rename updateJobStatus to something like updatedJob
to reflect it holds the returned job record, then replace the boolean check with
an explicit null or undefined check (e.g., if (!updatedJob)) to properly handle
the case where no row was updated and throw the error accordingly.
| interface BuildDeckForJobUseCaseOutput { | ||
| size: number; | ||
| key: string; | ||
| apkg: Buffer<ArrayBufferLike>; |
There was a problem hiding this comment.
Fix the Buffer type declaration.
The Buffer type in TypeScript doesn't accept generic parameters. It should be declared as just Buffer.
Apply this diff to fix the type:
- apkg: Buffer<ArrayBufferLike>;
+ apkg: Buffer;📝 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.
| apkg: Buffer<ArrayBufferLike>; | |
| apkg: Buffer; |
🤖 Prompt for AI Agents
In src/usecases/jobs/BuildDeckForJobUseCase.ts at line 22, the type declaration
for apkg incorrectly uses a generic parameter with Buffer. Remove the generic
parameter and declare apkg simply as type Buffer.
| export interface CreateJobWorkSpaceUseCaseInput { | ||
| id: string; | ||
| owner: string; | ||
| api: NotionAPIWrapper; | ||
| jobRepository: JobRepository; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant jobRepository from input interface.
The jobRepository is already injected via the constructor and shouldn't be part of the input interface. This redundancy could lead to confusion and maintenance issues.
export interface CreateJobWorkSpaceUseCaseInput {
id: string;
owner: string;
api: NotionAPIWrapper;
- jobRepository: JobRepository;
}📝 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.
| export interface CreateJobWorkSpaceUseCaseInput { | |
| id: string; | |
| owner: string; | |
| api: NotionAPIWrapper; | |
| jobRepository: JobRepository; | |
| } | |
| export interface CreateJobWorkSpaceUseCaseInput { | |
| id: string; | |
| owner: string; | |
| api: NotionAPIWrapper; | |
| } |
🤖 Prompt for AI Agents
In src/usecases/jobs/CreateJobWorkSpaceUseCase.ts between lines 11 and 16,
remove the jobRepository property from the CreateJobWorkSpaceUseCaseInput
interface since jobRepository is already injected through the constructor. This
will eliminate redundancy and prevent confusion by keeping jobRepository
management consistent and centralized.
| const { ws, exporter, settings, bl, rules } = await createWorkSpace.execute( | ||
| { api, id, owner, jobRepository } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant jobRepository parameter.
The jobRepository is already injected into CreateJobWorkSpaceUseCase via its constructor and shouldn't be passed in the execute parameters.
const { ws, exporter, settings, bl, rules } = await createWorkSpace.execute(
- { api, id, owner, jobRepository }
+ { api, id, owner }
);📝 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 { ws, exporter, settings, bl, rules } = await createWorkSpace.execute( | |
| { api, id, owner, jobRepository } | |
| ); | |
| const { ws, exporter, settings, bl, rules } = await createWorkSpace.execute( | |
| { api, id, owner } | |
| ); |
🤖 Prompt for AI Agents
In src/lib/storage/jobs/helpers/performConversion.ts around lines 86 to 88,
remove the jobRepository parameter from the execute call on createWorkSpace
since jobRepository is already injected via the constructor of
CreateJobWorkSpaceUseCase. Update the execute call to only pass { api, id, owner
} without jobRepository.
| if (!hasInProgressJob) { | ||
| console.log(`job ${id} was not started. Job is already active.`); | ||
| return res ? res.redirect('/uploads') : null; |
There was a problem hiding this comment.
Fix incorrect log message.
The log message is backwards - it says "was not started" when hasInProgressJob is false, but the condition suggests the job is already active.
- console.log(`job ${id} was not started. Job is already active.`);
+ console.log(`job ${id} is already active or completed.`);🤖 Prompt for AI Agents
In src/lib/storage/jobs/helpers/performConversion.ts around lines 52 to 54, the
log message incorrectly states "job was not started" when hasInProgressJob is
false, but this condition means the job is already active. Update the log
message to correctly reflect that the job is already active or running, ensuring
the message matches the condition's meaning.
1 participant
Fixes this report: [ERROR] [2anki.net] - Undefined binding(s) detected
when compiling SELECT. Undefined column(s): [owner] query: select * from
"jobs" where "owner" = ?
Signed-off-by: Alexander Alemayhu alexander@alemayhu.com
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style
Chores