Fail configured builds on Firestore read failure (#115) - #119
Merged
Merged
Conversation
getBeers(), getVenues(), and getBeerBySlug() used plain getDocs(), which silently resolves from the offline cache when a build-time backend read fails — indistinguishable from a legitimately empty collection and able to deploy an empty catalog, empty venue list, altered FAQ copy, or beer pages as 404s. Extend the #104 contract to all public build-time reads: unconfigured builds (CI, local without .env.local) return the empty fallback without initializing Firebase; configured builds read via getDocsFromServer so a legitimately empty result stays valid while a real failure throws and fails the build. Query semantics, ordering, and mapping are unchanged. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Reviewer's GuideAll public build-time Firestore reads now skip Firebase entirely when unconfigured and use getDocsFromServer() when configured, causing real backend failures to fail the build while preserving valid empty results and existing page semantics. Tests and documentation were expanded to enforce and explain this contract. Sequence diagram for configured build-time Firestore readssequenceDiagram
participant Build
participant Helper
participant Firebase
participant Firestore
Build->>Helper: getBeers() / getVenues() / getBeerBySlug() / getBeerStaticParams()
Helper->>Firebase: hasFirebaseConfig()
Firebase-->>Helper: true
Helper->>Firebase: getFirebaseDb()
Firebase-->>Helper: Firestore database
Helper->>Firestore: getDocsFromServer(query)
alt read succeeds
Firestore-->>Helper: Snapshot, possibly empty
Helper-->>Build: Mapped data or null
else backend or permission failure
Firestore-->>Helper: Reject with error
Helper-->>Build: Build failure
end
Flow diagram for the build-time Firestore read contractflowchart TD
A[Build-time helper called] --> B{"hasFirebaseConfig()"}
B -->|No| C[Return empty fallback]
C --> D[Build continues without initializing Firebase]
B -->|Yes| E[getDocsFromServer]
E --> F{Firestore read result}
F -->|Success, including empty collection| G[Map and return data]
F -->|Backend, network, or permission failure| H[Reject and fail build]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/lib/firestore-build-reads.test.ts" line_range="62-65" />
<code_context>
+ fn: () => Promise<T>,
+ expected: T
+): Promise<void> {
+ const appsBefore = getApps().length;
+ const result = await withEnv({}, fn);
+ assert.deepEqual(result, expected);
+ assert.equal(getApps().length, appsBefore);
+}
+
</code_context>
<issue_to_address>
**nitpick (testing):** The unconfigured-build test compares the app count with a baseline that can already contain an initialized Firebase app, so later fallback tests pass even if the helper initializes or uses Firebase during the call. This does not reliably verify the documented no-initialization contract for `getVenues`, `getBeerBySlug`, and `getBeerStaticParams`.
**Triggers:** When a preceding configured-path test has initialized Firebase in the same test process.
**Suggested fix:** Terminate and delete Firebase apps before each unconfigured-path assertion, or assert that the helper does not create a new app from a known zero-app baseline.
</issue_to_address>Sourcery assessment
Approved.
Group the unconfigured-build tests before the configured-failure tests so the getApps() assertion proves no Firebase app was ever initialized, rather than comparing against a nonzero baseline once a configured test has run. Addresses a Sourcery review nitpick on the PR. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
sourcery-ai
Bot
dismissed
their stale review
September 23, 2026 21:46
Sourcery withdrew this approval because the latest commits introduced blocking findings.
This branch was successfully deployed
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
Issue #104 hardened beer slug enumeration (
getBeerStaticParams) against the Firebase Web SDK's silent offline-cache fallback, but three public build-time read paths still used plaingetDocs(). On a configured build where Firestore is unreachable or denies the read,getDocs()resolves an empty offline cache — indistinguishable from a legitimately empty collection — so the build would happily deploy an empty catalog, an empty venue list, altered/where-to-buyFAQ copy, beer URLs missing fromsitemap.xml, and legitimate beer detail pages as 404s until the next rebuild.This PR extends the established #104 contract to every public build-time read:
.env.local): each helper returns its empty fallback ([]ornull) without initializing Firebase — credential-free builds remain intentional, and now produce no SDK warnings at all.getDocsFromServer(), which never resolves from the offline cache. A genuinely empty collection is still a valid successful result; a real backend/network/permission failure rejects and fails the build.Query semantics, ordering, filtering, mapping, and return types are unchanged.
getBeers()andgetBeerStaticParams()continue to sharepublicBeersQuery()so catalog enumeration cannot drift.Closes #115
Changes
lib/beers.ts:getBeers()andgetBeerBySlug()now guard onhasFirebaseConfig()and read viagetDocsFromServer();getDocsimport removed. JSDoc updated to state the shared contract;getBeerBySlugkeepssnapshot.empty → null → notFound()for genuinely absent slugs.lib/venues.ts:getVenues()gets the same guard + authoritative read;getDocsimport removed.tests/lib/firestore-build-reads.test.ts(renamed frombeers-static-params.test.ts): expanded from 7 to 17 tests covering all four helpers — unconfigured fallbacks that assert no Firebase app is ever initialized, configured-failure rejections against a real (nonexistent-project) backend, and per-function source guards that fail if anyone reintroduces the cache-fallbackgetDocscall or drops the guard/mapping.docs/TECHNICAL.md+.github/workflows/ci.ymlcomment: updated to describe the uniform contract (unconfigured → deliberate empty fallback; configured success → authoritative data; configured failure → build failure).Verification
npm cinpm run check:react-versionsnpx tsc --noEmitnpm run lintnpm test— 329 tests / 103 suites, all passnpm run test:rules(needs Java 21+) — 29 tests / 7 suites, all passnpm run build— see belownpm run test:smoke— 116 Playwright tests pass against the production buildnpm run check:md-linksnpm audit --omit=dev— 0 vulnerabilitiesBuild contract verified end-to-end on this branch:
/beers/[slug]emits zero params, zero Firebase warnings (previouslyINVALID_ARGUMENTnoise).env.local)next buildexits 1 —Failed to collect page data for /beers/[slug],code: 'unavailable'Smoke suite also confirms unknown beer slugs still render the intended 404 (
/beers/definitely-not-a-real-beeraxe test).Risk / deployment notes
beers/venuescollection still deploys fine.components/admin-dashboard.tsx) are authenticated browser reads and intentionally untouched.Generated with Devin
Summary by Sourcery
Make public Firestore reads fail configured builds on backend errors while retaining credential-free empty fallbacks for unconfigured builds.
Bug Fixes:
Enhancements:
Documentation:
Tests: