Fix: Improve reported reasons why an expectation is not supported#300
Merged
Conversation
WalkthroughMultiple expectation handler functions now pass human-readable lookup context labels ('Source' or 'Target') to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/packageLoudnessScan.ts (1)
80-82:⚠️ Potential issue | 🟠 MajorUse the target lookup in the readiness check.
lookupTargetcurrently resolves sources again, so a bad/missing target can still reportready: trueand only fail later inworkOnExpectation.🐛 Proposed fix
- const lookupTarget = await lookupLoudnessSources(worker, exp) + const lookupTarget = await lookupLoudnessTargets(worker, exp) if (!lookupTarget.ready) return { ready: lookupTarget.ready, knownReason: lookupTarget.knownReason, reason: lookupTarget.reason }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/packageLoudnessScan.ts` around lines 80 - 82, lookupLoudnessSources is being called to get the target but later code re-runs source resolution causing a missing/bad target to still show ready:true; change the readiness check and subsequent use to reuse the lookupTarget result instead of re-resolving. Specifically, after calling lookupLoudnessSources(store its result in lookupTarget), use lookupTarget.ready/lookupTarget.knownReason/lookupTarget.reason for readiness and pass the resolved target (lookupTarget.target or equivalent field returned by lookupLoudnessSources) into workOnExpectation and any downstream logic instead of calling lookupLoudnessSources again so a missing/invalid target will correctly mark the expectation as not ready.shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/packageDeepScan.ts (1)
73-92:⚠️ Potential issue | 🟠 MajorSame bug as in
packageScan.ts: target lookup calls the sources helper.Line 81 uses
lookupDeepScanSourcesforlookupTarget; should belookupDeepScanTargets. The readiness check currently never validates the target accessor.🐛 Proposed fix
- const lookupTarget = await lookupDeepScanSources(worker, exp) + const lookupTarget = await lookupDeepScanTargets(worker, exp)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/packageDeepScan.ts` around lines 73 - 92, The target readiness check uses the wrong helper: replace the second call to lookupDeepScanSources with lookupDeepScanTargets so lookupTarget is obtained correctly; update the variable lookupTarget (used in the readiness return) to come from await lookupDeepScanTargets(worker, exp) and keep the subsequent readiness check and return signatures (ready, knownReason, reason) and downstream usage (e.g., tryPackageRead remains against lookupSource.handle) unchanged.shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/mediaFileThumbnail.ts (1)
190-202:⚠️ Potential issue | 🟠 MajorLikely copy-paste bug:
lookupTargetused inside the source-type guard.Lines 193-194 test
lookupTarget.accessor.type === AccessType.HTTP/HTTP_PROXYwhile the surrounding conditions on lines 191, 192, 195, 196 all checklookupSource.accessor.type. Because of this, HTTP/HTTP_PROXY sources never satisfy the source branch (they only happen to pass when the target is HTTP/HTTP_PROXY, which is unrelated). This is pre-existing but adjacent to this PR’s scope and worth fixing while you’re improving lookup semantics.🐛 Proposed fix
if ( (lookupSource.accessor.type === Accessor.AccessType.LOCAL_FOLDER || lookupSource.accessor.type === Accessor.AccessType.FILE_SHARE || - lookupTarget.accessor.type === Accessor.AccessType.HTTP || - lookupTarget.accessor.type === Accessor.AccessType.HTTP_PROXY || + lookupSource.accessor.type === Accessor.AccessType.HTTP || + lookupSource.accessor.type === Accessor.AccessType.HTTP_PROXY || lookupSource.accessor.type === Accessor.AccessType.FTP || lookupSource.accessor.type === Accessor.AccessType.S3) &&🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/mediaFileThumbnail.ts` around lines 190 - 202, The conditional that determines allowed source/target accessor types in mediaFileThumbnail uses lookupTarget.accessor.type for the HTTP/HTTP_PROXY checks but should be checking lookupSource.accessor.type; update the two occurrences of lookupTarget.accessor.type === Accessor.AccessType.HTTP and lookupTarget.accessor.type === Accessor.AccessType.HTTP_PROXY to lookupSource.accessor.type === Accessor.AccessType.HTTP and lookupSource.accessor.type === Accessor.AccessType.HTTP_PROXY so the source-type guard (the if that references lookupSource/accessor types) correctly recognizes HTTP and HTTP_PROXY sources in the mediaFileThumbnail expectation handler.shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/packageScan.ts (1)
64-83:⚠️ Potential issue | 🟠 MajorBug: target lookup calls the sources helper.
Line 72 assigns
lookupScanSourcestolookupTarget, soisExpectationReadyToStartWorkingOnnever actually validates the target accessor — it checks sources twice. With this PR’s goal of surfacing better unsupported-accessor reasons, this is exactly the kind of defect that masks the improvements. Pre-existing, but in-scope to fix.🐛 Proposed fix
- const lookupTarget = await lookupScanSources(worker, exp) + const lookupTarget = await lookupScanTargets(worker, exp)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/packageScan.ts` around lines 64 - 83, The code mistakenly calls lookupScanSources twice (assigning both lookupSource and lookupTarget from lookupScanSources) so the target accessor is never validated; update the second call in the block where lookupTarget is assigned (used later in isExpectationReadyToStartWorkingOn flow) to call the correct helper for targets (e.g., lookupScanTargets(worker, exp)) so lookupTarget.ready/knownReason/reason reflect the target lookup result rather than the source.shared/packages/worker/src/worker/workers/genericWorker/packageContainerExpectationHandler.ts (1)
96-108:⚠️ Potential issue | 🟠 MajorOff-by-one: cronjob writability check uses wrong threshold
The condition
Object.keys(packageContainer.cronjobs).length > 1skips write permission checks for containers with exactly one cronjob, despite the comment stating "If no cronjobs are setup, no need to check writability" (which would imply> 0).The codebase shows single-cronjob configurations are valid (e.g.,
mediaFileConvert.tscreates containers with only acleanupcronjob). These cleanup operations require write access, so the current logic will incorrectly fail permission checks.Change the condition to
> 0:Fix
- if (Object.keys(packageContainer.cronjobs).length > 1) { + if (Object.keys(packageContainer.cronjobs).length > 0) { checks.write = true checks.writePackageContainer = true }Additionally, update the comment to reflect the intent: "If cronjobs are setup, check writability" or similar.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/packages/worker/src/worker/workers/genericWorker/packageContainerExpectationHandler.ts` around lines 96 - 108, The cronjob writability guard in the forWhat === 'cronjob' branch is off-by-one: change the condition from Object.keys(packageContainer.cronjobs).length > 1 to > 0 so single-cronjob containers trigger checks.write and checks.writePackageContainer; update the preceding comment to reflect the intent (e.g., "If cronjobs are set up, check writability") and keep references to packageContainer.cronjobs, checks.write, checks.writePackageContainer, and the forWhat === 'cronjob' branch so the write checks run for containers with exactly one cronjob.shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/lib.ts (1)
187-198:⚠️ Potential issue | 🟠 MajorRun the support check before constructing the handle.
This still creates the accessor handle before rejecting accessors unsupported by this worker. If handle construction validates worker-specific paths/config, an unsuitable accessor can throw before the loop reaches the next candidate.
Proposed fix
// See if the file is available at any of the targets: for (const { packageContainer, accessorId, accessor } of prioritizedAccessors) { - const handle = getAccessorHandle<Metadata>( - worker, - accessorId, - accessor, - accessorContext, - expectationContent, - expectationWorkOptions - ) const staticHandle = getAccessorStaticHandle(accessor) // Check that the accessor is supported by the worker: if (!staticHandle.doYouSupportAccess(worker, accessor)) { errorReasons.push({ @@ }) continue // Maybe next accessor works? } + + const handle = getAccessorHandle<Metadata>( + worker, + accessorId, + accessor, + accessorContext, + expectationContent, + expectationWorkOptions + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/lib.ts` around lines 187 - 198, The code constructs getAccessorHandle(...) before checking support; change the order to first obtain the static handle via getAccessorStaticHandle(accessor), call staticHandle.doYouSupportAccess(worker, accessor) and if it returns false skip/continue for that accessor, and only then call getAccessorHandle(worker, accessorId, accessor, accessorContext, expectationContent, expectationWorkOptions) to avoid constructing handles for unsupported accessors.
🧹 Nitpick comments (2)
apps/package-manager/packages/generic/src/generateExpectations/nrk/expectations-lib.ts (1)
241-285: Consider centralizing the proxy-source/depends logic.The repeated
useOnlyTargetsAsSources ? [...] : [...]anddependsOnFulfilledpattern looks correct, but a small helper would reduce the risk of these generators drifting later.Also applies to: 293-342, 352-400, 409-453, 566-613, 623-669
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/package-manager/packages/generic/src/generateExpectations/nrk/expectations-lib.ts` around lines 241 - 285, Extract the repeated logic that chooses whether to use only targets as sources and whether to set dependsOnFulfilled into a small helper used by all generators (e.g., create a function referenced from these files like resolveSourcesAndDepends or computeStartSourcesAndDepends that accepts the Expectation object and returns { startSources, dependsOnFulfilled } based on expectation.type === Expectation.Type.FILE_COPY_PROXY and expectation.startRequirement / expectation.endRequirement); then replace occurrences of the ternary expressions in startRequirement.sources and the conditional dependsOnFulfilled arrays with calls to that helper (update callers in the blocks around useOnlyTargetsAsSources, startRequirement, and dependsOnFulfilled at the listed ranges).shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/lib.ts (1)
153-155: Simplify the redundant lookup-reason type.
'Source' | 'Target' | stringis equivalent tostring, so the literal members do not add type safety. Consider using juststringif arbitrary values are allowed, or define a narrower explicit type'Source' | 'Target'if only these labels should be permitted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/lib.ts` around lines 153 - 155, The lookupAccessorHandles function's lookupReason parameter uses the redundant type "'Source' | 'Target' | string"; replace that with a single, appropriate type: either plain string if arbitrary values are allowed, or a narrower union type (e.g., type LookupReason = 'Source' | 'Target') to enforce only those two labels, then update the function signature lookupAccessorHandles<Metadata>(lookupReason: LookupReason, worker: BaseWorker) and adjust any callers to satisfy the chosen type; ensure imports/types are added where needed and run type-checks to confirm no downstream errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@shared/packages/expectationManager/src/expectationTracker/lib/trackedExpectationAPI.ts`:
- Around line 60-67: The code is mutating the caller-owned reason object when
appending the worker id; instead create a new reason object (e.g., a shallow or
deep clone) before modifying it so callers' objects aren't changed, then compare
that clonedReason to trackedExp.reason with _.isEqual and assign
trackedExp.reason = clonedReason and set updatedReason = true only when they
differ; ensure the mutation happens on the clonedReason and reference
trackedExp.session.assignedWorker.id when building the appended string.
In
`@shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/lib.ts`:
- Around line 214-222: The user-facing error string built when pushing into
errorReasons contains an extra closing parenthesis; update the construction
inside the errorReasons.push so the user message in basicResult.reason.user
reads `There is an issue with the Package: ${basicResult.reason.user}` (remove
the stray ")" ), ensuring you adjust the object created in that push
(referencing packageContainer, accessor, accessorId, basicResult) and keep the
tech message unchanged.
---
Outside diff comments:
In
`@shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/lib.ts`:
- Around line 187-198: The code constructs getAccessorHandle(...) before
checking support; change the order to first obtain the static handle via
getAccessorStaticHandle(accessor), call staticHandle.doYouSupportAccess(worker,
accessor) and if it returns false skip/continue for that accessor, and only then
call getAccessorHandle(worker, accessorId, accessor, accessorContext,
expectationContent, expectationWorkOptions) to avoid constructing handles for
unsupported accessors.
In
`@shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/mediaFileThumbnail.ts`:
- Around line 190-202: The conditional that determines allowed source/target
accessor types in mediaFileThumbnail uses lookupTarget.accessor.type for the
HTTP/HTTP_PROXY checks but should be checking lookupSource.accessor.type; update
the two occurrences of lookupTarget.accessor.type === Accessor.AccessType.HTTP
and lookupTarget.accessor.type === Accessor.AccessType.HTTP_PROXY to
lookupSource.accessor.type === Accessor.AccessType.HTTP and
lookupSource.accessor.type === Accessor.AccessType.HTTP_PROXY so the source-type
guard (the if that references lookupSource/accessor types) correctly recognizes
HTTP and HTTP_PROXY sources in the mediaFileThumbnail expectation handler.
In
`@shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/packageDeepScan.ts`:
- Around line 73-92: The target readiness check uses the wrong helper: replace
the second call to lookupDeepScanSources with lookupDeepScanTargets so
lookupTarget is obtained correctly; update the variable lookupTarget (used in
the readiness return) to come from await lookupDeepScanTargets(worker, exp) and
keep the subsequent readiness check and return signatures (ready, knownReason,
reason) and downstream usage (e.g., tryPackageRead remains against
lookupSource.handle) unchanged.
In
`@shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/packageLoudnessScan.ts`:
- Around line 80-82: lookupLoudnessSources is being called to get the target but
later code re-runs source resolution causing a missing/bad target to still show
ready:true; change the readiness check and subsequent use to reuse the
lookupTarget result instead of re-resolving. Specifically, after calling
lookupLoudnessSources(store its result in lookupTarget), use
lookupTarget.ready/lookupTarget.knownReason/lookupTarget.reason for readiness
and pass the resolved target (lookupTarget.target or equivalent field returned
by lookupLoudnessSources) into workOnExpectation and any downstream logic
instead of calling lookupLoudnessSources again so a missing/invalid target will
correctly mark the expectation as not ready.
In
`@shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/packageScan.ts`:
- Around line 64-83: The code mistakenly calls lookupScanSources twice
(assigning both lookupSource and lookupTarget from lookupScanSources) so the
target accessor is never validated; update the second call in the block where
lookupTarget is assigned (used later in isExpectationReadyToStartWorkingOn flow)
to call the correct helper for targets (e.g., lookupScanTargets(worker, exp)) so
lookupTarget.ready/knownReason/reason reflect the target lookup result rather
than the source.
In
`@shared/packages/worker/src/worker/workers/genericWorker/packageContainerExpectationHandler.ts`:
- Around line 96-108: The cronjob writability guard in the forWhat === 'cronjob'
branch is off-by-one: change the condition from
Object.keys(packageContainer.cronjobs).length > 1 to > 0 so single-cronjob
containers trigger checks.write and checks.writePackageContainer; update the
preceding comment to reflect the intent (e.g., "If cronjobs are set up, check
writability") and keep references to packageContainer.cronjobs, checks.write,
checks.writePackageContainer, and the forWhat === 'cronjob' branch so the write
checks run for containers with exactly one cronjob.
---
Nitpick comments:
In
`@apps/package-manager/packages/generic/src/generateExpectations/nrk/expectations-lib.ts`:
- Around line 241-285: Extract the repeated logic that chooses whether to use
only targets as sources and whether to set dependsOnFulfilled into a small
helper used by all generators (e.g., create a function referenced from these
files like resolveSourcesAndDepends or computeStartSourcesAndDepends that
accepts the Expectation object and returns { startSources, dependsOnFulfilled }
based on expectation.type === Expectation.Type.FILE_COPY_PROXY and
expectation.startRequirement / expectation.endRequirement); then replace
occurrences of the ternary expressions in startRequirement.sources and the
conditional dependsOnFulfilled arrays with calls to that helper (update callers
in the blocks around useOnlyTargetsAsSources, startRequirement, and
dependsOnFulfilled at the listed ranges).
In
`@shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/lib.ts`:
- Around line 153-155: The lookupAccessorHandles function's lookupReason
parameter uses the redundant type "'Source' | 'Target' | string"; replace that
with a single, appropriate type: either plain string if arbitrary values are
allowed, or a narrower union type (e.g., type LookupReason = 'Source' |
'Target') to enforce only those two labels, then update the function signature
lookupAccessorHandles<Metadata>(lookupReason: LookupReason, worker: BaseWorker)
and adjust any callers to satisfy the chosen type; ensure imports/types are
added where needed and run type-checks to confirm no downstream errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa4b91fb-4305-4d33-89e5-9b4d3eb1fb89
📒 Files selected for processing (20)
apps/package-manager/packages/generic/src/generateExpectations/nrk/expectations-lib.tsshared/packages/expectationManager/src/expectationTracker/lib/trackedExpectationAPI.tsshared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/fileCopy.tsshared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/fileCopyProxy.tsshared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/fileVerify.tsshared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/jsonDataCopy.tsshared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/kairosLoadToRam.tsshared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/lib.tsshared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/mediaFileConvert.tsshared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/mediaFilePreview.tsshared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/mediaFileThumbnail.tsshared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/packageDeepScan.tsshared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/packageIframesScan.tsshared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/packageLoudnessScan.tsshared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/packageScan.tsshared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/quantelClipCopy.tsshared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/quantelClipPreview.tsshared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/quantelClipThumbnail.tsshared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/renderHTML.tsshared/packages/worker/src/worker/workers/genericWorker/packageContainerExpectationHandler.ts
Comment on lines
+60
to
67
| if (reason && trackedExp.session.assignedWorker) { | ||
| // Add Worker id to tech reason: | ||
| reason.tech = `${reason.tech} (Worker: ${trackedExp.session.assignedWorker.id || 'N/A'})` | ||
| } | ||
|
|
||
| if (reason && !_.isEqual(trackedExp.reason, reason)) { | ||
| trackedExp.reason = reason | ||
| updatedReason = true |
There was a problem hiding this comment.
Avoid mutating the caller-owned reason object.
Line 62 mutates upd.reason in place. Several callers pass shared reason objects, so this can leak the worker suffix back to session/removal state, append it repeatedly on later updates, or mutate trackedExp.reason before the equality check so no status update is emitted.
Proposed fix
- if (reason && trackedExp.session.assignedWorker) {
+ const nextReason = reason ? { ...reason } : undefined
+
+ if (nextReason && trackedExp.session.assignedWorker) {
// Add Worker id to tech reason:
- reason.tech = `${reason.tech} (Worker: ${trackedExp.session.assignedWorker.id || 'N/A'})`
+ nextReason.tech = `${nextReason.tech} (Worker: ${trackedExp.session.assignedWorker.id || 'N/A'})`
}
- if (reason && !_.isEqual(trackedExp.reason, reason)) {
- trackedExp.reason = reason
+ if (nextReason && !_.isEqual(trackedExp.reason, nextReason)) {
+ trackedExp.reason = nextReason
updatedReason = true
}📝 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.
Suggested change
| if (reason && trackedExp.session.assignedWorker) { | |
| // Add Worker id to tech reason: | |
| reason.tech = `${reason.tech} (Worker: ${trackedExp.session.assignedWorker.id || 'N/A'})` | |
| } | |
| if (reason && !_.isEqual(trackedExp.reason, reason)) { | |
| trackedExp.reason = reason | |
| updatedReason = true | |
| const nextReason = reason ? { ...reason } : undefined | |
| if (nextReason && trackedExp.session.assignedWorker) { | |
| // Add Worker id to tech reason: | |
| nextReason.tech = `${nextReason.tech} (Worker: ${trackedExp.session.assignedWorker.id || 'N/A'})` | |
| } | |
| if (nextReason && !_.isEqual(trackedExp.reason, nextReason)) { | |
| trackedExp.reason = nextReason | |
| updatedReason = true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@shared/packages/expectationManager/src/expectationTracker/lib/trackedExpectationAPI.ts`
around lines 60 - 67, The code is mutating the caller-owned reason object when
appending the worker id; instead create a new reason object (e.g., a shallow or
deep clone) before modifying it so callers' objects aren't changed, then compare
that clonedReason to trackedExp.reason with _.isEqual and assign
trackedExp.reason = clonedReason and set updatedReason = true only when they
differ; ensure the mutation happens on the clonedReason and reference
trackedExp.session.assignedWorker.id when building the appended string.
Comment on lines
+214
to
+222
| errorReasons.push({ | ||
| reason: { | ||
| user: `There is an issue with the Package: ${basicResult.reason.user})`, | ||
| tech: `${packageContainer.label}: Accessor "${accessor.label || accessorId}": ${ | ||
| basicResult.reason.tech | ||
| }`, | ||
| }, | ||
| knownReason: basicResult.knownReason, | ||
| } | ||
| }) |
There was a problem hiding this comment.
Remove the stray parenthesis in the user-facing reason.
Line 216 currently emits messages like There is an issue with the Package: ... ).
Proposed fix
- user: `There is an issue with the Package: ${basicResult.reason.user})`,
+ user: `There is an issue with the Package: ${basicResult.reason.user}`,📝 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.
Suggested change
| errorReasons.push({ | |
| reason: { | |
| user: `There is an issue with the Package: ${basicResult.reason.user})`, | |
| tech: `${packageContainer.label}: Accessor "${accessor.label || accessorId}": ${ | |
| basicResult.reason.tech | |
| }`, | |
| }, | |
| knownReason: basicResult.knownReason, | |
| } | |
| }) | |
| errorReasons.push({ | |
| reason: { | |
| user: `There is an issue with the Package: ${basicResult.reason.user}`, | |
| tech: `${packageContainer.label}: Accessor "${accessor.label || accessorId}": ${ | |
| basicResult.reason.tech | |
| }`, | |
| }, | |
| knownReason: basicResult.knownReason, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@shared/packages/worker/src/worker/workers/genericWorker/expectationHandlers/lib.ts`
around lines 214 - 222, The user-facing error string built when pushing into
errorReasons contains an extra closing parenthesis; update the construction
inside the errorReasons.push so the user message in basicResult.reason.user
reads `There is an issue with the Package: ${basicResult.reason.user}` (remove
the stray ")" ), ensuring you adjust the object created in that push
(referencing packageContainer, accessor, accessorId, basicResult) and keep the
tech message unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
About Me
This pull request is posted on behalf of the NRK.
Type of Contribution
This is a: Feature / Code improvement
Current Behavior
If an expectation has multiple source/target packageContainer/Accessors and none fits, only the first reason why is reported back to user.
New Behavior
If an expectation has multiple source/target packageContainer/Accessors and none fits, all reasons why are included in the
techreason reported to the user.Status