feat: serve samples from the typescript server - #1257
Merged
igboyes merged 11 commits intoJul 27, 2026
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f82844edb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
igboyes
commented
Jul 23, 2026
Member
Author
|
Addressed the two Codex review threads (resolved):
(Replied here rather than inline because there's a pending review on the PR that would otherwise swallow the replies.) |
igboyes
commented
Jul 23, 2026
The sample detail queries for reads and artifacts filtered on sample_id only, so a legacy sample whose file rows are keyed by the text storage id (with sample_id null) appeared to have no read or artifact files. Match the delete path and OR against the storage id resolved from legacy_id.
reserveUploads only checked that each supplied upload existed and was unreserved, so a create-sample request could reserve reference or subtraction uploads, unfinished uploads, or removed ones — consuming unrelated files and scheduling a job that cannot process its inputs. Require each id to resolve to a visible reads upload (type reads, ready, not removed) before reserving.
The samples server functions were the last to use snake_case wire fields, inconsistent with the camelCase shapes every other migrated feature returns (references, jobs). Rename the sample, read, artifact, and rights fields to camelCase across the contract, the server data/functions layer, and the client consumers, and fold the search envelope onto the shared SearchResultV2. Consolidate the sample-specific job types too: a shared JobNested and JobState now live in @virtool/contracts, and a sample's embedded job is JobNested narrowed to the create_sample workflow that built it.
getSample and findSamples resolved the owner's handle in a second pass — a select over users keyed by the ids collected from the page — and getSample fetched the group with its own query. Both are one-to-one relationships, so join them onto the sample row instead, dropping getUsersByIds and getGroup. The collection relationships (labels, subtractions, reads, artifacts, and the workflow tags) stay as separate result sets.
user_id is nullable on legacy_samples, so a sample can exist with no creating user — an invalid state that getSample and getSampleOwnerId silently degraded to a blank, id-0 owner. Reading one now throws so the integrity violation surfaces instead of propagating a bogus owner. The sample list stays tolerant, so a single corrupt row cannot fail a whole page.
Drop the per-feature `types.ts` re-export shim for samples and references wire shapes; components now import Sample, Reference, and friends straight from @virtool/contracts instead of through a feature re-export. The shim only bought call-site stability during the earlier migration and cost a second place every contract type had to be listed. Also: - rename sample test mock helpers to match the server functions they stub (mockUpdateSample, mockDeleteSample, mockFindSamples, mockFindSamplePages, mockGetSample) - drop the dead `patchSample` wrapper and single-use `SampleID` type in samples/queries.ts and samples/types.ts - update docs/architecture.md and AGENTS.md to describe the new convention
The one-line summary said not to suffix exports with their layer but never mentioned how to resolve the collision that forces the question in the first place, which is the root cause `auth/functions.ts` drifted into `loginFn`-style exports instead of aliasing the colliding import.
…omain fn The prior commit had this backwards. loginFn established the actual convention first: a createServerFn export is an RPC call, not a plain function, so it keeps the Fn suffix to mark that at every call site. The plain name belongs to the domain function it wraps, which never crosses the network. getSample, getGroup, and the ~60 other exported server functions across src/server/*/functions.ts are the drift from this, not loginFn.
Every createServerFn-wrapped export now ends in Fn (getUser -> getUserFn, createSample -> createSampleFn, etc.) across all 15 src/server/*/functions.ts files that hadn't yet adopted the convention auth/functions.ts and getPasswordPolicyFn already followed. The suffix marks an RPC call at every call site; the domain function each one wraps keeps its plain name. Updates every call site (queries.ts modules, route guards, jobs/refresh.ts), every tests/server-fn/*.ts mock object and its consumers, and the functions.test.ts call()/callServerFn() string literals and authorization.test.ts's hardcoded exception list.
igboyes
force-pushed
the
igboyes/vir-2807-convert-sample-api-calls-to-server-functions
branch
from
July 27, 2026 17:52
bbebd95 to
8e423a2
Compare
igboyes
deleted the
igboyes/vir-2807-convert-sample-api-calls-to-server-functions
branch
July 27, 2026 18:03
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.
Summary
has_sample_rightpredicate exported for the Analyses work to reuse.legacy_samplesDrizzle mirror and move sample wire shapes into@virtool/contracts; addcreateJob/reserveUploadsso sample creation can schedule its job and hold its uploads.tests/api/samples.tsfakes totests/server-fn/samples.tsstubs, and mark Samples as Built indocs/database.md.