Skip to content

refactor(preview service): send results to REST API#5038

Draft
iainsproat wants to merge 8 commits intomainfrom
iain/preview-service-response-by-rest-api
Draft

refactor(preview service): send results to REST API#5038
iainsproat wants to merge 8 commits intomainfrom
iain/preview-service-response-by-rest-api

Conversation

@iainsproat
Copy link
Contributor

@iainsproat iainsproat commented Jul 4, 2025

Description & motivation

When designing the file import service, we simplified the pattern used by microservices when sending results. Instead of creating an additional queue and managing async message passing, we synchronously send the result to a REST API endpoint.

This refactor applies this same pattern to the preview service.

Changes:

  • token sent to the preview service requires stream:write permissions in addition to stream:read
  • additional endpoint POST /api/projects/:streamId/previews/jobs/:jobId/results to handle preview service responses
  • remove prometheus metric speckle_server_preview_jobs_response_queue_pending

To-do before merge:

Screenshots:

Validation of changes:

Checklist:

  • My pull request follows the guidelines in the Contributing guide?
  • My pull request does not duplicate any other open Pull Requests for the same update/change?
  • My commits are related to the pull request and do not amend unrelated code or documentation.
  • My code follows a similar style to existing code.
  • I have added appropriate tests.
  • I have updated or added relevant documentation.

References

- matches pattern used in file import service
- removes overhead of creating & managing a queue for responses
- REST AP requires streams:write to add preview
@iainsproat iainsproat requested a review from Copilot July 4, 2025 19:28
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the preview service to send results synchronously over HTTP instead of via a response queue, simplifying the messaging pattern.

  • Renamed the job payload field from responseQueue to responseUrl with descriptive Zod schemas
  • Removed response-queue handling code and introduced a new REST endpoint to receive and process preview results
  • Updated preview-service to POST results to the new endpoint and removed the response queue

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/shared/src/workers/previews/job.ts Renamed responseQueue to responseUrl and added .describe notes
packages/server/modules/previews/services/responses.ts Removed legacy response-queue consumer service
packages/server/modules/previews/services/createObjectPreview.ts Added Streams.Write scope and passed responseUrl
packages/server/modules/previews/resultListener.ts Dropped obsolete buildConsumePreviewResult and DB client logic
packages/server/modules/previews/rest/router.ts Added POST /results endpoint and removed queue-based handlers
packages/server/modules/previews/queues/previews.ts Simplified to request-only queue factory
packages/server/modules/previews/observability/metrics.ts Removed response-queue metrics
packages/server/modules/previews/index.ts Switched to single queue and wired in the new router
packages/server/modules/previews/domain/operations.ts Updated request types to include responseUrl
packages/server/modules/previews/clients/bull.ts Renamed createRequestAndResponseQueues to createRequestQueue
packages/preview-service/src/results.ts Implemented sendResult to POST over HTTP
packages/preview-service/src/main.ts Replaced response-queue logic with HTTP sender
Comments suppressed due to low confidence (4)

packages/server/modules/previews/rest/router.ts:354

  • The function getProjectDbClient is used here but not imported. Please add import { getProjectDbClient } from '@/modules/multiregion/utils/dbSelector' at the top of the file.
      const projectDb = await getProjectDbClient({ projectId })

packages/server/modules/previews/rest/router.ts:321

  • The function getStreamFactory is used here but not imported. Please add the appropriate import (e.g. import { getStreamFactory } from '@/modules/core/repositories/streams') at the top of the file.
        getStream: getStreamFactory({ db })

packages/server/modules/previews/rest/router.ts:2

  • [nitpick] The cors import is not used in this file. Consider removing this unused import to clean up the code.
import cors from 'cors'

packages/server/modules/previews/rest/router.ts:3

  • [nitpick] The validateScopes and authorizeResolver imports are not used in this file. Consider removing these unused imports.
import { validateScopes, authorizeResolver } from '@/modules/shared'

@codecov
Copy link

codecov bot commented Jul 4, 2025

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 58.67%. Comparing base (0f084ae) to head (990e001).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
packages/shared/src/workers/previews/job.ts 0.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5038      +/-   ##
==========================================
- Coverage   58.76%   58.67%   -0.09%     
==========================================
  Files         118      118              
  Lines        4637     4644       +7     
  Branches      531      531              
==========================================
  Hits         2725     2725              
- Misses       1912     1919       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants