StepNotPlanned as PnP Problem / Flatten AggregateErrors in GQL #3331
Conversation
|
Warning Rate limit exceeded@CarsonF has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve significant modifications across several components related to product progress handling. Key updates include enhanced error handling in the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
src/components/progress-report/migrations/reextract-all-progress-reports.migration.ts (1)
Line range hint
30-36: Consider enhancing error handling strategyGiven the PR's focus on improving StepNotPlanned error handling, consider these enhancements:
- Collect and aggregate errors across all failed extractions
- Differentiate between StepNotPlanned errors and other failures
- Add structured error reporting for better monitoring
Here's a suggested improvement:
+ private readonly failedExtractions: Array<{ + reportId: string; + fileVersionId: string; + error: Error; + }> = []; async up() { const session = this.fakeAdminSession; const pnps = createPaginator((page) => this.grabSomePnpsToReextract(page, 100), ); await asyncPool(2, pnps, async ({ dto: report, fv }) => { try { const pnp = this.files.asDownloadable(fv); const event = new PeriodicReportUploadedEvent(report, pnp, session); await this.db.conn.runInTransaction(() => this.eventBus.publish(event)); } catch (e) { + this.failedExtractions.push({ + reportId: report.id, + fileVersionId: fv.id, + error: e instanceof Error ? e : new Error(String(e)), + }); this.logger.error('Failed to re-extract PnP', { report: report.id, fvId: fv.id, exception: e, }); } }); + + if (this.failedExtractions.length > 0) { + this.logger.warn(`Migration completed with ${this.failedExtractions.length} failed extractions`, { + failures: this.failedExtractions.map(f => ({ + reportId: f.reportId, + fileVersionId: f.fileVersionId, + errorType: f.error.constructor.name, + message: f.error.message + })) + }); + } }src/components/product-progress/step-progress-extractor.service.ts (2)
42-42: Add JSDoc comment for the new cell property.The addition of the
cellproperty is a good improvement for error tracking. Consider adding documentation to explain its purpose and relationship to error handling.+ /** Reference to the source cell in the progress sheet, used for error context and reporting */ cell: Cell<ProgressSheet>;
42-42: Well-designed error handling enhancement.The addition of the cell reference creates a solid foundation for improved error handling:
- Enables precise error location reporting
- Facilitates error aggregation
- Maintains clean separation between extraction and error handling
Also applies to: 104-104
src/core/exception/exception.normalizer.ts (1)
137-155: Improve code documentation and error categorizationThe AggregateError handling implementation looks solid, but could benefit from these improvements:
- Replace the informal "shrug?" comment with a proper explanation of the code determination logic
- Consider adding a more specific code prefix for aggregated errors (e.g., 'AggregateError') to distinguish them from regular errors
Here's a suggested improvement:
return { errors, - // shrug? + // Determine the aggregate error type based on contained errors: + // - If all contained errors are client-side, mark as 'Client' + // - Otherwise, mark as 'Server' as it may contain server-side errors codes: [ + 'AggregateError', errors.every((e) => e.codes.has('Client')) ? 'Client' : 'Server', ], };src/components/product-progress/handlers/extract-pnp-progress.handler.ts (1)
104-145: Refactor error handling for better readabilityConsider extracting the error processing logic within the
catchblock into a separate function to improve readability and maintainability.Apply this diff to refactor the error handling:
await Promise.all( updates.map(async ({ extracted, ...input }) => { try { await this.progress.update( { ...input, reportId: event.report.id, variant: Progress.FallbackVariant, }, event.session, ); } catch (e) { if ( !( e instanceof AggregateError && e.message === 'Invalid Progress Input' ) ) { throw e; } + this.handleInvalidProgressInputErrors(e, extracted, result); } }), ); +private handleInvalidProgressInputErrors( + error: AggregateError, + extracted: ExtractedRowType, + result: ResultType, +) { + for (const err of error.errors) { + if (!(err instanceof StepNotPlannedException)) { + continue; + } + const stepLabel = ProductStep.entry(err.step).label; + const goalLabel = extracted.bookName ?? extracted.story; + result.addProblem({ + severity: 'Error', + groups: [ + 'Step is not planned', + `_${goalLabel}_ has progress reported on steps that have not been declared to be worked in this engagement`, + `_${goalLabel}_ has not declared _${stepLabel}_ as a step that will be worked in this engagement.`, + ], + message: stripIndent` + Please update the goal in CORD to mark this step as planned + or upload an updated PnP file to the "Planning Spreadsheet" on the engagement page. + `, + source: extracted.cell, + }); + } +}src/components/product-progress/product-progress.service.ts (1)
12-12: Consistent Use ofResourceLoaderfor Product DataThe introduction of
ResourceLoaderto load product data enhances consistency and maintainability. Ensure that all other parts of the codebase that load product data are updated to useResourceLoaderfor uniformity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/components/product-progress/handlers/extract-pnp-progress.handler.ts(4 hunks)src/components/product-progress/product-progress.repository.ts(0 hunks)src/components/product-progress/product-progress.service.ts(4 hunks)src/components/product-progress/step-progress-extractor.service.ts(2 hunks)src/components/progress-report/migrations/reextract-all-progress-reports.migration.ts(2 hunks)src/core/exception/exception.normalizer.ts(2 hunks)src/core/graphql/graphql-error-formatter.ts(3 hunks)
💤 Files with no reviewable changes (1)
- src/components/product-progress/product-progress.repository.ts
🔇 Additional comments (11)
src/components/progress-report/migrations/reextract-all-progress-reports.migration.ts (2)
11-11: LGTM: Migration version increment is appropriate
The migration version has been properly incremented to maintain unique versioning.
29-29: Verify transaction scope for event publishing
While wrapping the event publishing in a transaction is good for atomicity, consider whether the transaction scope should be expanded to include the entire PnP extraction process for each report.
Let's check if there are any other database operations in the event handler:
✅ Verification successful
Transaction scope for event publishing is adequate
Verified that no additional database operations occur within the PeriodicReportUploadedEvent handlers. The transaction correctly encapsulates the event publishing without missing operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for database operations in the PeriodicReportUploadedEvent handler
ast-grep --pattern 'class $_ implements EventHandler<PeriodicReportUploadedEvent> {
$$$
handle($_) {
$$$
}
}'
# Search for potential database operations in related services
rg -A 5 "PeriodicReportUploadedEvent"
Length of output: 10831
src/components/product-progress/step-progress-extractor.service.ts (1)
104-104: LGTM! Cell forwarding implementation looks good.
The cell property is correctly preserved in the common object, which will help with error context in both OBS and Written scripture scenarios.
src/core/exception/exception.normalizer.ts (3)
45-46: LGTM: Well-documented interface extension
The addition of the optional errors property to ExceptionJson is well-typed and documented, supporting the PR's objective to expose all errors instead of just the first occurrence.
52-52: LGTM: Documentation improvement
Fixed typo in the comment improving clarity.
137-155: Verify StepNotPlanned error handling
While the AggregateError handling looks correct, let's verify it properly handles StepNotPlanned errors as intended by the PR.
src/core/graphql/graphql-error-formatter.ts (1)
46-48: Confirm flatMap compatibility in the execution phase
As above, using flatMap may cause compatibility issues in environments running Node.js versions earlier than 11. Consider using map followed by flat(). Suggested change:
- const errors = result.errors.flatMap((error) =>
- this.formatError(error),
- );
+ const errors = result.errors.map((error) =>
+ this.formatError(error),
+ ).flat();src/components/product-progress/handlers/extract-pnp-progress.handler.ts (2)
142-142: Ensure 'extracted.cell' is defined when adding a problem
Verify that extracted.cell is defined to avoid potential issues when setting the source in result.addProblem.
Run the following script to check for undefined cell properties:
#!/bin/bash
# Description: Check for extracted rows without a 'cell' property
# Test: Search for extracted rows missing 'cell'. Expect: No matches.
jq '.[] | select(.cell == null)' progressRows.json130-130: Verify that 'goalLabel' is defined
Ensure that extracted.bookName or extracted.story is defined to prevent goalLabel from being undefined, which could lead to unclear error messages.
Run the following script to confirm that extracted.bookName or extracted.story is always defined:
src/components/product-progress/product-progress.service.ts (2)
157-174: Handle Aggregate Errors Effectively
Collecting validation errors and throwing an AggregateError improves error reporting by allowing multiple issues to be communicated at once. Ensure that this AggregateError is properly handled by the calling code so that all individual errors are presented to the user in a clear and actionable manner.
Run the following script to verify that AggregateError instances are caught and processed correctly elsewhere in the codebase:
121-121: Verify Proper Loading of Product with ResourceLoader
Using this.resources.load('Product', productId) replaces the previous method of obtaining product scope. Confirm that the loaded product includes all necessary properties required by subsequent code, especially if there are custom loaders or data transformations involved.
Run the following script to check how ResourceLoader is configured for the Product entity:
src/components/product-progress/handlers/extract-pnp-progress.handler.ts
Show resolved
Hide resolved
src/components/product-progress/handlers/extract-pnp-progress.handler.ts
Show resolved
Hide resolved
CarsonF
force-pushed
the
reextract-pnps
branch
from
00a6e80 to
160c82f
Compare
| if (!(error instanceof StepNotPlannedException)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This does swallow this error
cord-api-v3/src/components/product-progress/product-progress.service.ts
Lines 165 to 168 in 160c82f
I don't think that ProgressMeasurement.Number/Boolean is ever used really.
So the 100% cap is already enforced by the PnP.
So I don't think this case is ever hit in practice.
CarsonF
force-pushed
the
reextract-pnps
branch
from
160c82f to
81a400c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/components/product-progress/product-progress.service.ts (1)
157-174: Improved error handling with aggregationThe new error handling approach successfully implements the PR objective of collecting all StepNotPlanned errors instead of failing on the first one. However, the error collection logic could be more maintainable.
Consider extracting the validation logic into a separate method for better maintainability:
+ private validateStep( + step: ProductProgressInput['steps'][number], + index: number, + product: Product + ): Error[] { + const errors: Error[] = []; + + if (!product.steps.value.includes(step.step)) { + errors.push(new StepNotPlannedException(product.id, step.step, index)); + } + + if (step.completed && step.completed > (product.progressTarget.value ?? 0)) { + errors.push( + new InputException( + "Completed value cannot exceed product's progress target", + `steps.${index}.completed` + ) + ); + } + + return errors; + } async update(input: ProductProgressInput, session: Session) { const product = await this.resources.load('Product', input.productId); // ... authorization checks ... - const errors = input.steps.flatMap((step, index) => { - if (!product.steps.value.includes(step.step)) { - return new StepNotPlannedException(input.productId, step.step, index); - } - if ( - step.completed && - step.completed > (product.progressTarget.value ?? 0) - ) { - return new InputException( - "Completed value cannot exceed product's progress target", - `steps.${index}.completed`, - ); - } - return []; - }); + const errors = input.steps.flatMap((step, index) => + this.validateStep(step, index, product) + ); if (errors.length > 0) { throw new AggregateError(errors, 'Invalid Progress Input'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/components/product-progress/handlers/extract-pnp-progress.handler.ts(4 hunks)src/components/product-progress/product-progress.repository.ts(0 hunks)src/components/product-progress/product-progress.service.ts(4 hunks)src/components/product-progress/step-progress-extractor.service.ts(2 hunks)src/components/progress-report/migrations/reextract-all-progress-reports.migration.ts(2 hunks)src/core/exception/exception.normalizer.ts(2 hunks)src/core/graphql/graphql-error-formatter.ts(3 hunks)
💤 Files with no reviewable changes (1)
- src/components/product-progress/product-progress.repository.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/product-progress/handlers/extract-pnp-progress.handler.ts
- src/components/product-progress/step-progress-extractor.service.ts
- src/components/progress-report/migrations/reextract-all-progress-reports.migration.ts
- src/core/exception/exception.normalizer.ts
- src/core/graphql/graphql-error-formatter.ts
🔇 Additional comments (4)
src/components/product-progress/product-progress.service.ts (4)
43-43: LGTM: ResourceLoader injection
The addition of ResourceLoader as a dependency is a good architectural change, providing a more centralized way to load product data.
121-121: LGTM: Consistent use of ResourceLoader
The change to use ResourceLoader.load() is consistent with the new dependency injection and maintains proper error handling.
177-177: Skip comment about non-null assertion
146-149: LGTM: Consistent resource loading
The change to use ResourceLoader is consistent with the architectural changes and maintains proper authorization checks.
Ah...I think I can just revert that loader change. It turned out to be not necessary for this overall change. We can circle back later. |
When extracting from progress upload. Mutations from UI to update progress steps will still throw errors.
CarsonF
force-pushed
the
reextract-pnps
branch
from
81a400c to
66d6bb4
Compare
This exposes all StepNotPlanned errors, instead of just the first one.
For PnP progress upload, we convert these errors to extraction problems, instead of throwing.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
These updates improve user experience by providing clearer error messages and more robust data handling.