-
Notifications
You must be signed in to change notification settings - Fork 0
docs: #3 architecture decision records #36
Merged
Merged
+106
−0
Conversation
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
Add docs/architecture.md with six short decision records so peer reviewers and examiners can follow our reasoning. Each one covers context, options considered, the decision and its consequences: - PostgreSQL + GraphQL Yoga over MongoDB, Neo4j or a BaaS - keyset (cursor) pagination instead of OFFSET - sorting and filtering in SQL - per-request DataLoaders against N+1 - simulated users instead of login - plain CSS, Apollo Client cache and URL as search state Closes #3
robertky
reviewed
Sep 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small wording suggestion for decision 3: consider writing “parameterised SQL queries” instead of “one parameterised query”. Search results, totalCount, and genre facets may naturally require separate parameterised queries, without changing the decision to perform filtering and sorting in the database.
It would also be good to rebase the branch onto the current main afterwards, so the PR receives a CI run against the latest setup.
rachelks
reviewed
Sep 30, 2026
Reword three decisions after review: - keyset pagination: "paging stays efficient on later pages" instead of "every page is equally fast" - sorting and filtering: parameterised SQL queries, since results, totalCount and facets may be separate queries - Apollo cache: returning the album updates its cached data, but lists such as myCrate and reviews may still need a cache update or refetch Refs: #3
rachelks
approved these changes
Oct 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good after the small wording changes!
erikhfj
added a commit
that referenced
this pull request
Oct 1, 2026
…start Two bugs, both reported by rachelks after #36 merged and the deploy went red. 1. `systemctl list-unit-files | grep -q '^crate-api.service'` took the else branch even when the unit was installed. `grep -q` exits at the first match and closes the pipe, `systemctl` gets SIGPIPE and dies, and with `pipefail` the pipeline reports failure so the `if` falls through to "not installed yet, skipping". Whether it matched or not depended on how much output `systemctl` had flushed before grep closed, which is why it was intermittent. The log said: Failed to print table: Broken pipe crate-api.service not installed yet (arrives in M2). Skipping. Replaced with `[ -f /etc/systemd/system/crate-api.service ]`. No pipe, no SIGPIPE, no pipefail interaction. 2. `sudo systemctl restart crate-api` fails because the self-hosted runner has no TTY and no passwordless rule: sudo: A terminal is required to authenticate Needs a NOPASSWD rule scoped to exactly these commands. That is a change on the VM, not in this file, so it is called out in the step comment and in the PR. Also gated the restart on `server/` actually being in the repository. The unit on the VM still points at prototype code from 23 Sep, and `server/` does not exist on main until M2. Restarting unconditionally would bring that stale process back. Now it skips with a stated reason while M1 is the current state, and fails loudly if `server/` exists but was never built. Reported by rachelks. Co-Authored-By: opencode <noreply@opencode.ai>
Sign in
to join this conversation on GitHub.
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.
What this does
Documentation for #3: short architecture decision records, so peer reviewers and examiners can follow why we built Crate the way we did.
It is written from what the repo already says:
schema.graphql,docs/data-model.mdand the architecture rules inAGENTS.md. It was not copied fromarchive/scaffold-prototype.What was done
docs/architecture.mdhas six decisions. Each one has context, options considered, the decision and its consequences:OFFSETWhat was tested
Markdown only, so the relevant gate is formatting:
pnpm lint/typecheck/testdo not apply, because the PR adds no code.Notes
AGENTS.mdsays the simulated user id is sent "with every mutation". ButmyCrate,myReviewandisInCrateare queries, so the id has to go with those requests too. Decision 5 says "each request". Worth agreeing on before M2.Closes
Closes #3.