From 62685fe841e48c2e247d5510261960e5b988ab97 Mon Sep 17 00:00:00 2001 From: Duygu <84078855+duuyguyiilmaz@users.noreply.github.com> Date: Mon, 31 Aug 2026 16:34:08 +0300 Subject: [PATCH] docs: drop the unimplemented report feature and reframe base64 storage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two entries under "Known limitations" were not limitations. The report mechanism was never built. Carrying it as an open gap invited the reading that the app is unfinished, when in fact the feature is simply not part of it. Removed from the README, from the firestore.rules comment, and from the rules test that used it as an example. The test itself stays — that an unmatched path fails closed is a real security property, worth keeping whether or not `reports` ever existed — but it now asserts the general rule instead of naming a collection nobody will recognise. Base64 file storage is a decision, not an unresolved problem. Firebase Storage needs the Blaze plan; this project stays on Spark; encoding the file into the note document is how uploads work at all under that constraint. It moves to Key decisions alongside the other trade-offs. The ~650 KB ceiling stays documented word for word — it is the first thing a reviewer will ask about, and the app is built around the number rather than surprised by it: images are downscaled to fit, larger PDFs are rejected with a message that says why. Also removes the shebang from backfill-created-at.js. The repo normalises line endings, so a Windows checkout gives the file CRLF, and a shebang ending in \r is one esbuild does not strip — which made the test importing it fail to parse with a SyntaxError pointing at the import statement. CI never saw it, because Linux checkouts get LF; it only broke after the merged branch was checked out locally. The script is always invoked through node or the npm script, so the line bought nothing. A comment now says why it must not come back. Firestore suite: 40 tests pass. Android suite: 107. Co-Authored-By: Claude Opus 5 --- README.md | 20 ++++++++------------ firestore-tests/backfill-created-at.js | 6 +++++- firestore-tests/rules.test.js | 6 +++--- firestore.rules | 4 ++-- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 7cd43b7..51c9c44 100644 --- a/README.md +++ b/README.md @@ -102,6 +102,13 @@ flowchart TD transaction, so two people rating the same note at once cannot lose one of the votes. - **File content lives in a subcollection.** `notes/{id}/content/file` is separate from the note metadata so feed and leaderboard queries never download file data. +- **Files are base64 in Firestore, not in Storage.** Firebase Storage is the usual home + for binary data, but it requires the billed Blaze plan and this project stays on the + free Spark plan. Encoding the file into a Firestore document keeps uploads working + within that constraint, at a known cost: base64 inflates data by about a third and a + Firestore document may not exceed 1 MiB, so the practical ceiling is ~650 KB. The app + is built around that number rather than surprised by it — images are downscaled and + re-compressed to fit, and larger PDFs are rejected with a message that says why. - **Rating arithmetic is a pure function.** `RatingCalculator` has no Firebase dependency, which is what makes it unit-testable — see [Tests](#tests). - **Repositories are interfaces.** Each one has a single Firebase-backed implementation @@ -186,14 +193,6 @@ They are covered by their own test suite; see [Rules tests](#rules-tests). Honest list of things a reviewer would spot, and why they are the way they are. -**Note files are base64-encoded into Firestore, capping uploads at ~650 KB.** -The right answer is Firebase Storage, which stores binary data as-is with no such -limit. Storage requires the billed Blaze plan, and this project stays on the free -Spark plan, so files are base64-encoded into a Firestore document instead. Since -base64 inflates data by about a third and a Firestore document may not exceed 1 MiB, -the practical ceiling is ~650 KB. Images are downscaled and re-compressed to fit; -larger PDFs are rejected with a clear message. - **The points system is client-authoritative.** Rating a note updates the note's totals *and* the uploader's `points` directly from the client, so the security rules have to permit one user to modify another user's points. @@ -216,9 +215,6 @@ repeat a failed delete by tapping the button again, so the toast does not leave stuck — but the retry is still theirs to figure out. Offering it in the snackbar would mean carrying the note id through `ProfileActionState.DeleteError`. -**The report mechanism is unimplemented.** It is part of the original project brief but -there is no code for it, so the security rules deliberately deny the `reports` collection. - --- ## Getting started @@ -355,7 +351,7 @@ it down again; nothing touches the real project, and no billing account is invol | `notes` | `uploaderUid` cannot be forged on create; metadata edits are the uploader's alone; a rater may touch only `ratingSum`, `ratingCount` and `avgRating`; delete is owner-only. | | `note content` | The batched note-plus-file upload, owner-only replace and delete, and a test that deliberately asserts the *open* create rule, so the gap documented in `firestore.rules` cannot be closed by accident and go unnoticed. | | `ratings` | The `_` document id, which is what stops one user voting as another, plus the 1–5 integer range and the ban on deleting votes. | -| `unmatched paths` | A collection with no rule stays closed, including the `reports` collection mentioned in this README but never implemented. | +| `unmatched paths` | A path no `match` block covers is denied, so adding a collection to the app without adding a rule fails closed rather than open. | Fixtures are seeded with `withSecurityRulesDisabled`, so a broken rule surfaces as a failed assertion rather than a failed setup. Each suite asserts both directions — the diff --git a/firestore-tests/backfill-created-at.js b/firestore-tests/backfill-created-at.js index 40b4381..249afd9 100644 --- a/firestore-tests/backfill-created-at.js +++ b/firestore-tests/backfill-created-at.js @@ -1,7 +1,11 @@ -#!/usr/bin/env node /** * Normalises every note's `createdAt` to a Firestore Timestamp. * + * Deliberately has no `#!` line. The repo normalises to CRLF on Windows checkouts, and + * a shebang ending in `\r` is one esbuild does not strip, so the test that imports this + * file failed to parse — on Windows only, which is why CI stayed green. The script is + * always invoked through `node` or the npm script, so the shebang bought nothing. + * * Why this exists: paginating the feed means ordering on `createdAt` in the query * instead of sorting in the client, and Firestore's ordering has two behaviours that * turn stale data into user-visible damage. diff --git a/firestore-tests/rules.test.js b/firestore-tests/rules.test.js index 89d0a46..0639295 100644 --- a/firestore-tests/rules.test.js +++ b/firestore-tests/rules.test.js @@ -367,8 +367,8 @@ describe("ratings", () => { describe("unmatched paths", () => { test("a collection with no rule is closed even to signed-in users", async () => { - // README mentions a `reports` collection that was never implemented. - await assertFails(getDoc(doc(rater, "reports", "anything"))); - await assertFails(setDoc(doc(rater, "reports", "anything"), { note: NOTE })); + // Adding a collection to the app without adding a rule for it must fail closed. + await assertFails(getDoc(doc(rater, "unruled", "anything"))); + await assertFails(setDoc(doc(rater, "unruled", "anything"), { note: NOTE })); }); }); diff --git a/firestore.rules b/firestore.rules index 43149d8..144ec2e 100644 --- a/firestore.rules +++ b/firestore.rules @@ -111,7 +111,7 @@ service cloud.firestore { allow delete: if false; } - // Yukarıda eşleşmeyen her yol reddedilir. README'de geçen `reports` - // koleksiyonu henüz uygulanmadı, o yüzden kasten kural tanımlanmadı. + // Yukarıda eşleşmeyen her yol reddedilir. Yeni bir koleksiyon eklendiğinde + // kuralı da yazılana kadar kapalı kalır — açık kalmasındansa erişilemez olması. } }