gergo/previews #3765
gergo/previews #3765
Conversation
There was a problem hiding this comment.
What's the _bak directory? Is it a backup to be removed?
| @@ -1,4 +1,4 @@ | |||
| import type { Preview } from '@/domain/domain.js' | |||
| import type { Preview } from '../domain/domain.js' | |||
There was a problem hiding this comment.
Can we retain the alias pattern please? i.e. @/domain/domain.js would be preferable.
| @@ -12,7 +12,7 @@ import { | |||
| notifyUpdateFactory, | |||
| updatePreviewMetadataFactory | |||
| } from '@/repositories/objectPreview.js' | |||
| import { insertPreviewFactory } from '@/repositories/previews.js' | |||
| import { insertPreviewFactory } from '../repositories/previews.js' | |||
There was a problem hiding this comment.
Again, the alias pattern should be used.
| import type { InsertPreview } from '@/repositories/previews.js' | ||
| import type { GeneratePreview } from '../clients/previewService.js' | ||
| import type { Angle, ObjectIdentifier, PreviewId } from '../domain/domain.js' | ||
| import type { InsertPreview } from '../repositories/previews.js' |
There was a problem hiding this comment.
alias pattern should be used. Is there an eslint rule missing?
|
|
| COPY --link --from=build-stage /speckle-server/packages/objectloader ./objectloader | ||
| COPY --link --from=build-stage /speckle-server/packages/viewer ./viewer | ||
| COPY --link --from=build-stage /speckle-server/packages/preview-service ./preview-service | ||
| COPY --from=build-stage /speckle-server/packages/shared ./shared |
| const jobId = job.jobId | ||
| const jobLogger = logger.child({ jobId }) | ||
| const start = new Date() | ||
| jobLogger.info({ start }, 'Picking up job {jobId}') |
There was a problem hiding this comment.
Do we need the { start } here, as the timestamp of the log entry is effectively the start time?
| process.on('SIGINT', async () => { | ||
| logger.info('Received signal to shut down') | ||
| browser.close() | ||
| server.close(() => { |
There was a problem hiding this comment.
on next request to the same missing preview, the server is going to create a new preview job.
If this is true; is there a chance that if multiple users try to preview a new model, then multiple jobs for the same preview will be added to the queue? And will the completion of the first in the queue cause the cancellation of subsequent jobs in the queue (or in-flight/ being rendered)?
packages/shared/src/previews/job.ts
Outdated
| export type JobPayload = z.infer<typeof jobPayload> | ||
|
|
||
| const previewResult = z.object({ | ||
| duration: z.number(), |
There was a problem hiding this comment.
ideally would have units in the property name, or at least mentioned in a .description message.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3765 +/- ##
==========================================
- Coverage 71.21% 70.88% -0.34%
==========================================
Files 401 405 +4
Lines 18168 18339 +171
Branches 2961 2972 +11
==========================================
+ Hits 12939 12999 +60
- Misses 4231 4342 +111
Partials 998 998 ☔ View full report in Codecov by Sentry. |
- histogram metric should be summary - error responses include duration in seconds - attempt to remove metric before adding it (prevent errors with duplicate metrics)
Description & motivation
Limitations with the existing preview service:
This PR aims to resolve these shortcomings
Changes:
To-do before merge:
Screenshots:
Validation of changes:
Checklist:
References