Implement Book Identity System and cleanup documentation - #1
Conversation
- Implemented `grid_counters` table and `increment_counter` RPC in `schema.sql`. - Added `lib/geohash.ts` for Projected GeoHash (US/Global). - Added `lib/id_generator.ts` for bijective map ID generation. - Updated `app/generate/page.tsx` with user-friendly messages. - Cleaned up documentation: archived requirements as `book-identity-system.md` in `docs/completed`, removed implementation plan.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive book identity system that replaces the previous hash-based approach with a location-based counter system. The new system generates unique book IDs by combining a geohash-based location prefix with an obfuscated sequential suffix.
Key changes:
- Adds database infrastructure with
grid_counterstable and atomicincrement_counterRPC function - Implements custom projected geohash encoding for US and global locations with distinct handling
- Creates bijective mapping algorithm to obfuscate sequential counters into compact Base32 suffixes
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| schema.sql | Adds grid_counters table and increment_counter RPC function for atomic counter management |
| lib/geohash.ts | Implements custom geohash encoding with US-specific bounding box and global fallback |
| lib/id_generator.ts | Implements bijective mapping to generate obfuscated book ID suffixes from sequential counters |
| app/api/books/generate/route.ts | Simplifies API route to use new generateBookId function |
| app/generate/page.tsx | Updates user-facing messages from "Hit recorded" to "ID Generated" |
| docs/completed/book-identity-system.md | Archives requirements and implementation retrospective |
| .gitignore | Adds VS Code workspace files to ignore list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function toBase32(num: number, length: number): string { | ||
| let result = ""; | ||
| for (let i = 0; i < length; i++) { | ||
| result = BASE32[num % 32] + result; | ||
| num = Math.floor(num / 32); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
The toBase32 function doesn't handle the case where the input number is too large for the specified length. If counter * PRIME overflows or produces a value that needs more than the specified number of characters, the extra digits will be lost. Consider adding a validation check or a comment explaining the expected input range constraints.
| // or extend the cipher. | ||
| // Let's extend the cipher lightly to 20 bits. | ||
| const PRIME_20 = 486187; // Large prime fitting in 20 bits | ||
| const XOR_20 = 786433; |
There was a problem hiding this comment.
The PRIME_20 value (486187) is larger than 20 bits can hold. A 20-bit value can only represent 0 to 1,048,575, but 486187 is within this range. However, when multiplying by counter values, the product may overflow 20 bits before the mask is applied. This could lead to loss of information in the bijective mapping. Consider documenting the maximum supported counter value or adjusting the algorithm to ensure the multiplication doesn't overflow before masking.
| const XOR_20 = 786433; | |
| const XOR_20 = 786433; | |
| // The maximum counter value to avoid overflow before masking: | |
| // (counter * PRIME_20) < 2^20 => counter < floor(2^20 / PRIME_20) | |
| const MAX_COUNTER_20BIT = Math.floor(0x100000 / PRIME_20); // 2^20 = 0x100000 | |
| if (counter > MAX_COUNTER_20BIT) { | |
| throw new Error( | |
| `Counter value ${counter} exceeds maximum supported for 20-bit bijective mapping (${MAX_COUNTER_20BIT}).` | |
| ); | |
| } |
| export function encodeGeoHash(latitude: number, longitude: number): string { | ||
| // Check if within Continental US | ||
| if ( | ||
| latitude >= US_LAT_MIN && | ||
| latitude <= US_LAT_MAX && | ||
| longitude >= US_LON_MIN && | ||
| longitude <= US_LON_MAX | ||
| ) { | ||
| return encode( | ||
| latitude, | ||
| longitude, | ||
| US_LAT_MIN, | ||
| US_LAT_MAX, | ||
| US_LON_MIN, | ||
| US_LON_MAX, | ||
| 5 | ||
| ); | ||
| } else { | ||
| // Rest of World: Prefix 'W' + 4 char standard global hash | ||
| // Standard Bounds: Lat -90..90, Lon -180..180 | ||
| const suffix = encode(latitude, longitude, -90, 90, -180, 180, 4); | ||
| return "W" + suffix; | ||
| } | ||
| lat[2] = (lat[0] + lat[1]) / 2; | ||
| lon[2] = (lon[0] + lon[1]) / 2; | ||
|
|
||
| return { latitude: lat, longitude: lon }; | ||
| } |
There was a problem hiding this comment.
Missing input validation for latitude and longitude parameters. The function should validate that latitude is between -90 and 90, and longitude is between -180 and 180 before proceeding with encoding. Invalid coordinates could produce incorrect geohash values.
| -- Note: Insert/Update is handled via RPC, security definer function recommended if we want to restrict direct table access, | ||
| -- but for now we'll allow authenticated users to potentially read. | ||
| -- Writing should be done via the function to ensure atomicity. |
There was a problem hiding this comment.
The table has read access enabled for all users but lacks explicit policies for INSERT and UPDATE operations. While the comment mentions using the RPC function for writes, without explicit policies restricting direct table access, authenticated users could potentially bypass the atomic increment function and write directly to the table, causing data inconsistencies. Consider adding explicit DENY policies for INSERT and UPDATE, or document that the function should be created with SECURITY DEFINER to control access.
| -- Note: Insert/Update is handled via RPC, security definer function recommended if we want to restrict direct table access, | |
| -- but for now we'll allow authenticated users to potentially read. | |
| -- Writing should be done via the function to ensure atomicity. | |
| -- DENY direct INSERT and UPDATE to grid_counters; only allow via function. | |
| CREATE POLICY "Deny insert for all users" ON public.grid_counters FOR INSERT WITH CHECK (false); | |
| CREATE POLICY "Deny update for all users" ON public.grid_counters FOR UPDATE USING (false); |
| // Helper to normalize user input (O->0, I/L->1) | ||
| export function normalizeGeoHash(input: string): string { | ||
| return input | ||
| .toUpperCase() | ||
| .replace(/O/g, "0") | ||
| .replace(/[IL]/g, "1") | ||
| .replace(/[^0-9A-Z]/g, ""); // Strip non-alphanumeric | ||
| } |
There was a problem hiding this comment.
The normalization function replaces 'O' with '0' and 'I'/'L' with '1', but the BASE32 alphabet used (Crockford Base32) doesn't include 'I', 'L', or 'O'. The comment suggests this is for normalizing user input, but there's potential confusion: the normalization maps to characters that might not be valid in the actual BASE32 alphabet being used. Consider documenting whether this is intentional (for user input forgiveness) or if the mapping should align with the actual BASE32 alphabet.
| CREATE OR REPLACE FUNCTION public.increment_counter(prefix_in TEXT) | ||
| RETURNS INTEGER | ||
| LANGUAGE plpgsql | ||
| AS $$ | ||
| DECLARE | ||
| new_value INTEGER; | ||
| BEGIN | ||
| INSERT INTO public.grid_counters (prefix, counter) | ||
| VALUES (prefix_in, 1) | ||
| ON CONFLICT (prefix) | ||
| DO UPDATE SET | ||
| counter = grid_counters.counter + 1, | ||
| updated_at = now() | ||
| RETURNING counter INTO new_value; | ||
|
|
||
| RETURN new_value; | ||
| END; | ||
| $$; |
There was a problem hiding this comment.
The increment_counter function doesn't explicitly set a transaction isolation level or use explicit locking. While PostgreSQL's default ON CONFLICT handling provides atomicity for single operations, in high-concurrency scenarios with the same prefix, there could be performance implications. Consider documenting the expected concurrency behavior or adding a comment about the transaction semantics to help future maintainers understand the guarantees provided.
| let is_even = true; | ||
| let i = 0; | ||
| let lat: number[] = []; | ||
| let lon: number[] = []; | ||
| let lat_interval = [minLat, maxLat]; | ||
| let lon_interval = [minLon, maxLon]; | ||
| let bit = 0; | ||
| let ch = 0; | ||
| let precision = 12; | ||
| let geohash = ""; | ||
|
|
||
| lat[0] = 25.0; | ||
| lat[1] = 50.0; | ||
| lon[0] = -125.0; | ||
| lon[1] = -65.0; | ||
|
|
||
| while (geohash.length < precision) { | ||
| if (is_even) { | ||
| let mid = (lon[0] + lon[1]) / 2; | ||
| const mid = (lon_interval[0] + lon_interval[1]) / 2; | ||
| if (longitude > mid) { | ||
| ch |= BITS[bit]; | ||
| lon[0] = mid; | ||
| } else lon[1] = mid; | ||
| lon_interval[0] = mid; | ||
| } else { | ||
| lon_interval[1] = mid; | ||
| } | ||
| } else { | ||
| let mid = (lat[0] + lat[1]) / 2; | ||
| const mid = (lat_interval[0] + lat_interval[1]) / 2; | ||
| if (latitude > mid) { | ||
| ch |= BITS[bit]; | ||
| lat[0] = mid; | ||
| } else lat[1] = mid; | ||
| lat_interval[0] = mid; | ||
| } else { | ||
| lat_interval[1] = mid; | ||
| } | ||
| } | ||
|
|
||
| is_even = !is_even; |
There was a problem hiding this comment.
Inconsistent naming convention: the function uses snake_case (is_even, lat_interval, lon_interval) while the rest of the TypeScript codebase typically uses camelCase. Consider renaming to isEven, latInterval, and lonInterval for consistency with TypeScript naming conventions.
| import { supabase } from "./supabase"; | ||
| import { encodeGeoHash, BASE32 } from "./geohash"; | ||
|
|
||
| // Constants for encryption |
There was a problem hiding this comment.
The comment says "Constants for encryption" but these are actually constants for obfuscation, not encryption. The bijective mapping is a form of encoding/obfuscation, not cryptographic encryption. Consider changing this to "Constants for obfuscation" or "Constants for bijective mapping" to be more accurate.
| // Constants for encryption | |
| // Constants for obfuscation (bijective mapping) |
| export async function generateBookId(lat: number, lon: number): Promise<string> { | ||
| // 1. Calculate Prefix | ||
| const prefix = encodeGeoHash(lat, lon); | ||
|
|
||
| // 2. Get Atomic Counter from DB | ||
| const { data, error } = await supabase.rpc("increment_counter", { | ||
| prefix_in: prefix, | ||
| }); | ||
|
|
||
| if (error) { | ||
| console.error("Error generating ID counter:", error); | ||
| throw new Error("Failed to generate Book ID"); | ||
| } | ||
|
|
||
| const counter = data as number; | ||
|
|
||
| // 3. Generate Suffix | ||
| const suffix = generateSuffix(counter); | ||
|
|
||
| // 4. Combine | ||
| return `${prefix}-${suffix}`; | ||
| } |
There was a problem hiding this comment.
Missing input validation for latitude and longitude parameters. The function should validate that lat is between -90 and 90, and lon is between -180 and 180 before processing. Invalid coordinates could lead to unexpected behavior in the geohash encoding.
| const { | ||
| location: { lat, long }, | ||
| } = await request.json(); | ||
| const locationString = `${lat},${long}`; | ||
| const hash = crypto | ||
| .createHash("sha256") | ||
| .update(locationString) | ||
| .digest("hex"); | ||
| let num = BigInt("0x" + hash); | ||
|
|
||
| let code = ""; | ||
| for (let i = 0; i < LENGTH; i++) { | ||
| const index = Number(num % BigInt(DISAMBIGUATED_CHARS.length)); | ||
| code += DISAMBIGUATED_CHARS[index]; | ||
| num = num / BigInt(DISAMBIGUATED_CHARS.length); | ||
| } | ||
| // const cookieStore = cookies(); | ||
| // const supabase = createRouteHandlerClient({ cookies: () => cookieStore }); | ||
| const code = await generateBookId(lat, long); |
There was a problem hiding this comment.
Missing input validation for the request body. The code assumes the request will have the expected structure with location.lat and location.long properties. If the request body is malformed or missing these properties, the destructuring will fail or pass undefined values to generateBookId. Consider adding validation to check that lat and long are present and are valid numbers before processing.
- Relaxed type checking for route params in app/api/books/[id]/route.ts to resolve Next.js 13.5 build error. - Temporarily disabled generateStaticParams to prevent build-time database connection failures.
- ensures atomic increments while preventing direct table writes by authenticated users.
- Throws error if coordinates are out of valid range (-90..90, -180..180).
- Renamed is_even -> isEven - Renamed lat_interval -> latInterval - Renamed lon_interval -> lonInterval
449d9b8 to
997d4bf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Geohash library for Javascript (Modified for LFL Tracker) | ||
| // Original: (c) 2008 David Troy | ||
| // Distributed under the MIT License | ||
|
|
There was a problem hiding this comment.
The documentation states that Crockford Base32 is used, but the BASE32 constant uses a different character set than Crockford's specification. Crockford Base32 uses "0123456789ABCDEFGHJKMNPQRSTVWXYZ" (lowercase) but specifically uses lowercase letters in its standard. The implementation appears to be using an uppercase variant. Consider either updating the documentation to clarify this is an uppercase Crockford-style alphabet, or verify that this character set matches the intended specification.
| // Uses Crockford Base32 alphabet (uppercase variant: "0123456789ABCDEFGHJKMNPQRSTVWXYZ") | |
| // Uses Crockford Base32 alphabet (uppercase variant: "0123456789ABCDEFGHJKMNPQRSTVWXYZ") |
| // Helper to normalize user input (O->0, I/L->1) | ||
| export function normalizeGeoHash(input: string): string { | ||
| return input | ||
| .toUpperCase() | ||
| .replace(/O/g, "0") | ||
| .replace(/[IL]/g, "1") | ||
| .replace(/[^0-9A-Z]/g, ""); // Strip non-alphanumeric |
There was a problem hiding this comment.
The normalization function replaces 'I' and 'L' with '1', but the BASE32 alphabet doesn't contain 'I' or 'L' (they're intentionally excluded). This suggests the function is designed to handle user input errors, but it doesn't handle 'U' which is also missing from the BASE32 alphabet (should likely map to 'V'). Consider adding a comment explaining this is for user input normalization and potentially handling all excluded characters consistently.
| // Helper to normalize user input (O->0, I/L->1) | |
| export function normalizeGeoHash(input: string): string { | |
| return input | |
| .toUpperCase() | |
| .replace(/O/g, "0") | |
| .replace(/[IL]/g, "1") | |
| .replace(/[^0-9A-Z]/g, ""); // Strip non-alphanumeric | |
| // Helper to normalize user input for geohash. | |
| // Maps commonly confused/excluded BASE32 characters to their likely intended equivalents: | |
| // O->0, I/L->1, U->V. Strips non-alphanumeric characters. | |
| export function normalizeGeoHash(input: string): string { | |
| return input | |
| .toUpperCase() | |
| .replace(/O/g, "0") | |
| .replace(/[IL]/g, "1") | |
| .replace(/U/g, "V") | |
| .replace(/[^0-9A-Z]/g, ""); |
| - `XOR_KEY`: 14325 | ||
| - `MODULUS`: 32768 (15-bit, 3 chars in Base32) | ||
| - **Overflow Handling**: | ||
| - If counter > 32768, logic automatically expands to a 4-character suffix using a larger prime/modulus (20-bit). |
There was a problem hiding this comment.
The condition checks if counter < MODULUS_15BIT (32768), but the documentation states "If counter > 32768" for overflow. This is inconsistent - when counter equals exactly 32768, the 3-character encoding will be used, but 32768 doesn't fit in 15 bits (max is 32767). The condition should be counter <= 32767 or counter < 32768 to match, but the logic is correct since any value >= 32768 will use 4 characters. Consider updating the documentation to say "counter >= 32768" for clarity.
| - If counter > 32768, logic automatically expands to a 4-character suffix using a larger prime/modulus (20-bit). | |
| - If counter >= 32768, logic automatically expands to a 4-character suffix using a larger prime/modulus (20-bit). |
| if (isEven) { | ||
| const mid = (lonInterval[0] + lonInterval[1]) / 2; | ||
| if (longitude > mid) { | ||
| ch |= BITS[bit]; | ||
| lon[0] = mid; | ||
| } else lon[1] = mid; | ||
| lonInterval[0] = mid; | ||
| } else { | ||
| lonInterval[1] = mid; | ||
| } |
There was a problem hiding this comment.
The function doesn't handle the edge case where latitude or longitude exactly equals the midpoint during the encoding process. When longitude equals mid, the else branch is taken (setting lonInterval[1] = mid), which means equal values are treated as less than the midpoint. While this is consistent behavior, it could be worth documenting this edge case handling or considering whether >= comparison might be more intuitive.
| export async function GET( | ||
| request: Request, | ||
| { params }: { params: { code: string } } | ||
| { params }: any |
There was a problem hiding this comment.
The params type is declared as 'any' which bypasses TypeScript's type safety. This should be properly typed to ensure type safety and better developer experience. Consider defining a proper type like '{ params: { id: string } }' or using Next.js's built-in type helpers.
| { params }: any | |
| { params }: { params: { id: string } } |
| ### Success Criteria Met | ||
| - [x] **Uniqueness**: IDs are guaranteed unique per location via DB sequence. | ||
| - [x] **Locality**: Prefix `W6PSK` correctly identifies a specific neighborhood. | ||
| - [x] **Usability**: IDs are short (8 chars) and use a typo-resistant alphabet. |
There was a problem hiding this comment.
The documentation claims a typo-resistant alphabet is used, but it references Crockford Base32 which excludes certain confusing characters. However, the normalizeGeoHash function doesn't handle all potentially confusing characters. For instance, 'U' is not in the BASE32 alphabet but isn't normalized. Consider documenting which specific characters are intentionally excluded and how user input errors are handled, or extending the normalization to be comprehensive.
| - Identifies the specific neighborhood grid cell (~0.4 mile resolution). | ||
| - **Algorithm**: Custom Projected GeoHash (Z-order curve) using Crockford Base32. | ||
| - **Projections**: | ||
| - **Continental US**: Custom bounding box (Lat 24-50, Lon -125 to -66) for ~0.4 mile resolution. |
There was a problem hiding this comment.
The comment states longitude range as "-125 to -66", but the actual constant US_LON_MAX is -66.0. The eastern edge of the continental US is actually around -66 to -67 degrees (Maine), so -66 is approximately correct. However, for consistency and accuracy, consider verifying these bounds match the intended coverage area, as standard continental US bounds are often cited as approximately -125 to -66 or -124 to -66.
| - **Continental US**: Custom bounding box (Lat 24-50, Lon -125 to -66) for ~0.4 mile resolution. | |
| - **Continental US**: Custom bounding box (Lat 24-50, Lon -125 to -66; matches code constants) for ~0.4 mile resolution. |
- addressed PR comments regarding fencepost errors and boundary testing expectations.
- Updated terminology (Encryption -> Obfuscation) - Expanded TEST_CASES.md to cover: - 20-bit Prime overflow safety analysis - Input normalization rationale - RLS security boundary verification
grid_counterstable andincrement_counterRPC inschema.sql.lib/geohash.tsfor Projected GeoHash (US/Global).lib/id_generator.tsfor bijective map ID generation.app/generate/page.tsxwith user-friendly messages.book-identity-system.mdindocs/completed, removed implementation plan.