Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR replaces the OpenLibrary autocomplete system with Google Books search for book generation and enhances the API's metadata parsing to support both data sources. The changes modernize the book selection UI with a cleaner interface while maintaining backward compatibility with existing OpenLibrary data structures.
- Switched from OpenLibrary to Google Books API for book search
- Simplified the UI by removing form-based inputs in favor of a search component
- Enhanced API metadata parsing to handle both Google Books and OpenLibrary formats
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| components/google-book-search.tsx | Added authors field to the book selection callback interface |
| app/layout.tsx | Attempted to mock font variables but introduced incorrect CSS variable names that will break styling |
| app/generate/page.tsx | Complete refactor from form-based OpenLibrary autocomplete to Google Books search with improved UX |
| app/api/books/generate/route.ts | Enhanced metadata parsing logic to support both Google Books and OpenLibrary data formats with flexible field mapping |
Comments suppressed due to low confidence (3)
app/layout.tsx:21
- Unused variable inter.
const inter = { variable: 'font-sans' };
app/layout.tsx:22
- Unused variable crimson.
const crimson = { variable: 'font-serif' };
app/generate/page.tsx:22
- Unused variable locationError.
const { latitude, longitude, error: locationError } = useLocation();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }); | ||
|
|
||
| // Helper to get cookie | ||
| // Helper to get cookie (reused from original) |
There was a problem hiding this comment.
The comment says "(reused from original)" but this function was actually already present in the original code. While accurate, this comment adds no value and could be removed for cleaner code.
| // Helper to get cookie (reused from original) |
| // Reset generated code if they search again? | ||
| // Actually keeping it simple: just select. |
There was a problem hiding this comment.
The inline comment suggests uncertainty about the implementation choice ("Reset generated code if they search again? Actually keeping it simple: just select."). This comment should either be removed if the decision is final, or the behavior should be clarified. Consider removing this implementation note as it doesn't add value for future maintainers.
| // Reset generated code if they search again? | |
| // Actually keeping it simple: just select. |
| // The 'book' object might be an OpenLibraryDoc or a generic object from GoogleBookSearch | ||
| const bookData = book as any; | ||
|
|
||
| const title = bookData.title || "Unknown Title"; |
There was a problem hiding this comment.
The metadata parsing code lacks validation for the incoming book data. While fallbacks handle missing fields, there's no check that the book object exists or has minimum required data. This could result in books being created with all default values ("Unknown Title", "Unknown Author") if malformed data is sent.
Consider adding validation before parsing to ensure the book object has at least a title field.
| body: JSON.stringify({ | ||
| book: selectedBook, | ||
| location: { | ||
| lat: latitude ? latitude : 0, | ||
| long: longitude ? longitude : 0, | ||
| lat: latitude, | ||
| long: longitude, | ||
| }, | ||
| anonymousId, | ||
| } as GenerateBookCodeRequest), | ||
| }), |
There was a problem hiding this comment.
The GenerateBookCodeRequest type still expects an OpenLibraryDoc for the book property, but the API now receives book objects from GoogleBookSearch with a different structure (title, authors, coverUrl). This type definition is now incorrect and should be updated to accept either format or a more generic book type.
This type mismatch could cause TypeScript errors or confusion for developers.
|
|
||
| return () => clearTimeout(debounceTimer); // Cleanup the timer | ||
| }, [titleInput, isbnInput]); | ||
| const { latitude, longitude, error: locationError } = useLocation(); |
There was a problem hiding this comment.
The variable locationError is destructured from the useLocation hook but never used. Consider removing it if error handling isn't needed, or implement proper error handling to inform users if location access fails.
| const { latitude, longitude, error: locationError } = useLocation(); | |
| const { latitude, longitude } = useLocation(); |
| // Mock font variables to avoid breaking layout | ||
| const inter = { variable: 'font-sans' }; | ||
| const crimson = { variable: 'font-serif' }; | ||
|
|
There was a problem hiding this comment.
The mock font objects are still being destructured and passed to the className, but they no longer provide the expected properties. Since the body className has been simplified to just font-sans, the inter and crimson variables are now unused and should be removed entirely.
These variables serve no purpose if they're not being added to the className attribute.
| // Mock font variables to avoid breaking layout | |
| const inter = { variable: 'font-sans' }; | |
| const crimson = { variable: 'font-serif' }; |
| const cover_url = typedBook ? getBookCover(typedBook) : null; | ||
| const isbn = typedBook?.isbn ? typedBook.isbn[0] : null; | ||
| // The 'book' object might be an OpenLibraryDoc or a generic object from GoogleBookSearch | ||
| const bookData = book as any; |
There was a problem hiding this comment.
The type safety has been lost by using any type. While the flexible metadata parsing is good for backward compatibility, consider creating a union type that represents both GoogleBook and OpenLibraryDoc formats instead of using any.
This would provide type safety while still handling both data sources, and make it clearer what properties are expected.
| // The 'book' object might be an OpenLibraryDoc or a generic object from GoogleBookSearch | ||
| const bookData = book as any; | ||
|
|
||
| const title = bookData.title || "Unknown Title"; | ||
|
|
||
| // Handle authors: could be 'authors' (Google) or 'author_name' (OpenLibrary) or just 'author' | ||
| let author = "Unknown Author"; | ||
| if (Array.isArray(bookData.authors) && bookData.authors.length > 0) { | ||
| author = bookData.authors.join(", "); | ||
| } else if (Array.isArray(bookData.author_name) && bookData.author_name.length > 0) { | ||
| author = bookData.author_name[0]; // Keep first author for consistency with old behavior | ||
| } else if (typeof bookData.author === 'string') { | ||
| author = bookData.author; | ||
| } | ||
|
|
||
| // Handle cover: use direct url or generate from OLID | ||
| let cover_link = bookData.coverUrl || bookData.cover_url; | ||
| if (!cover_link && bookData.cover_edition_key) { | ||
| cover_link = getBookCover(bookData); | ||
| } | ||
|
|
||
| const isbn = bookData.isbn ? (Array.isArray(bookData.isbn) ? bookData.isbn[0] : bookData.isbn) : null; |
There was a problem hiding this comment.
The new metadata parsing logic introduced in the API route lacks test coverage. Given that this repository has existing test coverage for API routes (e.g., tests/app/api/books/[id]/route.test.ts), the new logic that handles both Google Books and OpenLibrary formats should be tested to ensure:
- Correct parsing of Google Books format (authors array, coverUrl)
- Backward compatibility with OpenLibrary format (author_name, cover_edition_key)
- Proper fallback to default values
- Correct handling of edge cases (missing fields, different data structures)
| long: z.string().min(2, "Title must be at least 2 characters"), | ||
| isbn: z.string().min(2, "ISBN must be at least 8 characters"), | ||
| }); | ||
| import { GenerateBookCodeRequest } from "@/lib/types"; |
There was a problem hiding this comment.
Unused import GenerateBookCodeRequest.
| import { GenerateBookCodeRequest } from "@/lib/types"; |
| title: "Location Required", | ||
| description: "We need your location to generate a code.", | ||
| variant: "destructive" | ||
| }) |
There was a problem hiding this comment.
Avoid automated semicolon insertion (92% of all statements in the enclosing function have an explicit semicolon).
| }) | |
| }); |
…ook generation and enhance metadata parsing in the API.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 10 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { cookies } from "next/headers" | ||
| import { createClient } from "@supabase/supabase-js" | ||
| import { parseBookMetadata } from "@/lib/book-utils" | ||
| import { OpenLibraryDoc, getBookCover } from "@/lib/openLibrary" |
There was a problem hiding this comment.
The imports OpenLibraryDoc and getBookCover are no longer used directly in this file after the refactoring. The parseBookMetadata helper now handles the OpenLibrary logic internally. These unused imports should be removed to keep the code clean.
| import { OpenLibraryDoc, getBookCover } from "@/lib/openLibrary" |
| const olBook: BookMetadata = { | ||
| title: "OL Book Title", | ||
| author_name: ["OL Author"], | ||
| cover_edition_key: "OL123M", | ||
| isbn: ["0987654321"], | ||
| key: "/works/OL123W", | ||
| // Force type to satisfy simplified mock if needed, but the structure matches | ||
| edition_key: ["OL123M"], | ||
| publish_year: [2021], | ||
| } as any // Cast as any because OpenLibraryDoc has many required fields |
There was a problem hiding this comment.
The OpenLibraryDoc type definition has all fields marked as required, but the test mock casts to any because some fields (like key, edition_key, publish_year) are not part of the actual type definition. This indicates a mismatch between the actual OpenLibrary API response structure and the type definition. Consider making optional fields in OpenLibraryDoc optional (e.g., cover_edition_key?, isbn?) to better reflect the actual data structure and avoid the need for unsafe type casts.
| // Generic fallback if author is string (legacy/stub) | ||
| else if ("author" in bookData && typeof (bookData as any).author === "string") { | ||
| author = (bookData as any).author | ||
| } | ||
|
|
||
| // Cover | ||
| if ("coverUrl" in bookData && bookData.coverUrl) { | ||
| cover_url = bookData.coverUrl | ||
| } else if ("cover_url" in bookData && bookData.cover_url) { | ||
| cover_url = (bookData as any).cover_url |
There was a problem hiding this comment.
The function uses (bookData as any).author to access a property that doesn't exist on either GoogleBookData or OpenLibraryDoc types. This fallback for legacy/stub data should either be removed if no longer needed, or properly documented if there's a specific data format that requires this handling.
| // Generic fallback if author is string (legacy/stub) | |
| else if ("author" in bookData && typeof (bookData as any).author === "string") { | |
| author = (bookData as any).author | |
| } | |
| // Cover | |
| if ("coverUrl" in bookData && bookData.coverUrl) { | |
| cover_url = bookData.coverUrl | |
| } else if ("cover_url" in bookData && bookData.cover_url) { | |
| cover_url = (bookData as any).cover_url | |
| // Generic fallback if author is string (legacy/stub data format) | |
| else if ("author" in bookData) { | |
| const legacyAuthor = (bookData as { author?: unknown }).author | |
| if (typeof legacyAuthor === "string") { | |
| author = legacyAuthor | |
| } | |
| } | |
| // Cover | |
| if ("coverUrl" in bookData && bookData.coverUrl) { | |
| cover_url = bookData.coverUrl | |
| } else if ("cover_url" in bookData) { | |
| const legacyCoverUrl = (bookData as { cover_url?: string | null }).cover_url | |
| if (legacyCoverUrl) { | |
| cover_url = legacyCoverUrl | |
| } |
| if (!cover_url && "cover_edition_key" in bookData) { | ||
| // Cast is safe here because we checked for cover_edition_key presence which implies OpenLibraryDoc structure |
There was a problem hiding this comment.
The getBookCover function doesn't handle cases where cover_edition_key might be falsy or invalid. If the OpenLibrary API returns invalid keys, this will generate broken image URLs. Consider adding validation or returning null for invalid inputs to make error handling more predictable.
| if (!cover_url && "cover_edition_key" in bookData) { | |
| // Cast is safe here because we checked for cover_edition_key presence which implies OpenLibraryDoc structure | |
| if ( | |
| !cover_url && | |
| "cover_edition_key" in bookData && | |
| typeof (bookData as any).cover_edition_key === "string" && | |
| (bookData as any).cover_edition_key.trim() !== "" | |
| ) { | |
| // Cast is safe here because we checked for a valid, non-empty cover_edition_key which implies OpenLibraryDoc structure |
| global.Request = class Request { | ||
| constructor(input, init) { | ||
| return { url: input, input, init, json: () => Promise.resolve({}) } | ||
| } |
There was a problem hiding this comment.
The simplified Request mock returns a plain object instead of a proper Request instance. This could cause issues if code tries to access Request-specific methods or properties like headers, method, or clone(). Consider using a more complete mock or ensuring that all necessary Request interface members are included.
| global.Request = class Request { | |
| constructor(input, init) { | |
| return { url: input, input, init, json: () => Promise.resolve({}) } | |
| } | |
| class SimpleHeaders { | |
| constructor(headers = {}) { | |
| this._headers = {} | |
| for (const key of Object.keys(headers)) { | |
| this._headers[key.toLowerCase()] = String(headers[key]) | |
| } | |
| } | |
| get(name) { | |
| return this._headers[name.toLowerCase()] ?? null | |
| } | |
| set(name, value) { | |
| this._headers[name.toLowerCase()] = String(value) | |
| } | |
| } | |
| global.Request = class Request { | |
| constructor(input, init = {}) { | |
| const isRequestLike = input && typeof input === "object" && "url" in input | |
| this.url = typeof input === "string" ? input : isRequestLike ? input.url : "" | |
| this.method = init.method || (isRequestLike ? input.method : "GET") | |
| const baseHeaders = | |
| init.headers || | |
| (isRequestLike && input.headers) || | |
| {} | |
| const HeadersImpl = global.Headers || SimpleHeaders | |
| this.headers = baseHeaders instanceof (global.Headers || SimpleHeaders) | |
| ? baseHeaders | |
| : new HeadersImpl(baseHeaders) | |
| this.body = init.body || (isRequestLike ? input.body : null) | |
| this.signal = init.signal || null | |
| } | |
| clone() { | |
| return new Request(this.url, { | |
| method: this.method, | |
| headers: this.headers, | |
| body: this.body, | |
| signal: this.signal, | |
| }) | |
| } | |
| async json() { | |
| if (this.body == null) return {} | |
| if (typeof this.body === "string") { | |
| try { | |
| return JSON.parse(this.body) | |
| } catch { | |
| return {} | |
| } | |
| } | |
| return this.body | |
| } |
| import { parseBookMetadata } from "@/lib/book-utils" | ||
| import { BookMetadata } from "@/lib/types" | ||
|
|
||
| describe("parseBookMetadata", () => { | ||
| it("should parse Google Books data correctly", () => { | ||
| const googleBook: BookMetadata = { | ||
| title: "Google Book Title", | ||
| authors: ["Author One", "Author Two"], | ||
| coverUrl: "http://example.com/cover.jpg", | ||
| isbn: "1234567890", | ||
| googleId: "g1", | ||
| } | ||
|
|
||
| const result = parseBookMetadata(googleBook) | ||
|
|
||
| expect(result).toEqual({ | ||
| title: "Google Book Title", | ||
| author: "Author One, Author Two", | ||
| cover_url: "http://example.com/cover.jpg", | ||
| isbn: "1234567890", | ||
| }) | ||
| }) | ||
|
|
||
| it("should parse OpenLibrary data correctly", () => { | ||
| const olBook: BookMetadata = { | ||
| title: "OL Book Title", | ||
| author_name: ["OL Author"], | ||
| cover_edition_key: "OL123M", | ||
| isbn: ["0987654321"], | ||
| key: "/works/OL123W", | ||
| // Force type to satisfy simplified mock if needed, but the structure matches | ||
| edition_key: ["OL123M"], | ||
| publish_year: [2021], | ||
| } as any // Cast as any because OpenLibraryDoc has many required fields | ||
|
|
||
| const result = parseBookMetadata(olBook) | ||
|
|
||
| expect(result).toEqual({ | ||
| title: "OL Book Title", | ||
| author: "OL Author", | ||
| cover_url: "https://covers.openlibrary.org/b/olid/OL123M-M.jpg", // Expecting getBookCover logic | ||
| isbn: "0987654321", | ||
| }) | ||
| }) | ||
|
|
||
| it("should handle missing optional fields gracefully", () => { | ||
| const minimalBook: BookMetadata = { | ||
| title: "Minimal Book", | ||
| } as any | ||
|
|
||
| const result = parseBookMetadata(minimalBook) | ||
|
|
||
| expect(result).toEqual({ | ||
| title: "Minimal Book", | ||
| author: "Unknown Author", | ||
| cover_url: null, | ||
| isbn: null, | ||
| }) | ||
| }) | ||
|
|
||
| it("should return defaults for null input", () => { | ||
| const result = parseBookMetadata(null as any) | ||
| expect(result).toEqual({ | ||
| title: "Unknown Title", | ||
| author: "Unknown Author", | ||
| cover_url: null, | ||
| isbn: null, | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The test file only tests the parseBookMetadata utility function, but doesn't test the actual POST API route handler. The route handler contains important business logic including authentication, book code generation, database insertions, and error handling that should be covered by tests. Consider adding integration tests for the POST handler similar to the pattern used in __tests__/app/api/sightings/claim/route.test.ts.
| } else if ("cover_url" in bookData && bookData.cover_url) { | ||
| cover_url = (bookData as any).cover_url |
There was a problem hiding this comment.
The function uses (bookData as any).cover_url to access a property that doesn't exist on either GoogleBookData or OpenLibraryDoc types. This suggests there might be legacy code or inconsistent data structures. If this fallback is no longer needed after migrating to Google Books, it should be removed. Otherwise, document what format requires this fallback.
| } else if ("cover_url" in bookData && bookData.cover_url) { | |
| cover_url = (bookData as any).cover_url | |
| } else if ("cover_url" in bookData) { | |
| // Legacy/alternate formats (e.g. some OpenLibrary-style responses) may expose `cover_url` in snake_case | |
| const legacyCoverUrl = (bookData as { cover_url?: unknown }).cover_url | |
| if (typeof legacyCoverUrl === "string" && legacyCoverUrl) { | |
| cover_url = legacyCoverUrl | |
| } |
| if (typeof global.Response === "undefined") { | ||
| global.Response = class Response { | ||
| constructor(body, init = {}) { | ||
| this.body = body | ||
| this.status = init.status || 200 | ||
| this.statusText = init.statusText || "" | ||
| this.headers = new global.Headers(init.headers) | ||
| this.ok = this.status >= 200 && this.status < 300 | ||
| } | ||
|
|
||
| async json() { | ||
| return JSON.parse(this.body) | ||
| } | ||
|
|
||
| async text() { | ||
| return this.body | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (!Response.json) { | ||
| Response.json = function (data, init) { | ||
| return new Response(JSON.stringify(data), { | ||
| ...init, | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| ...(init && init.headers), | ||
| }, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| if (typeof global.Request === "undefined") { | ||
| global.Request = class Request { | ||
| constructor(input, init = {}) { | ||
| this.url = input | ||
| this.method = init.method || "GET" | ||
| this.headers = new global.Headers(init.headers) | ||
| this.body = init.body | ||
| } | ||
|
|
||
| async json() { | ||
| return JSON.parse(this.body) | ||
| constructor(body, init) { | ||
| return { | ||
| body, | ||
| init, | ||
| ok: true, | ||
| status: init?.status || 200, | ||
| headers: { | ||
| get: (key) => init?.headers?.[key] || null, | ||
| }, | ||
| json: () => Promise.resolve(body), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The simplified Response mock returns a plain object instead of a proper Response instance. This could break tests if code uses Response-specific methods like clone(), arrayBuffer(), text(), or checks instanceof Response. The mock also doesn't properly implement the headers interface - it only has a get method but is missing other Headers methods like set, has, forEach, etc.
| import { OpenLibraryDoc, getBookCover } from "./openLibrary" | ||
|
|
||
| /** | ||
| * Helper to parse metadata from different book sources (Google vs OpenLibrary) |
There was a problem hiding this comment.
The function is missing JSDoc documentation for its parameters and return type. Adding documentation would help clarify what formats are accepted (GoogleBookData vs OpenLibraryDoc) and what the returned object structure contains (title, author, cover_url, isbn).
| * Helper to parse metadata from different book sources (Google vs OpenLibrary) | |
| * Helper to parse metadata from different book sources (Google vs OpenLibrary). | |
| * | |
| * @param {BookMetadata} bookData - Raw book metadata, typically from Google Books or an OpenLibraryDoc. | |
| * @returns {{ title: string; author: string; cover_url: string | null; isbn: string | null }} Parsed metadata | |
| * including normalized title, author, cover URL, and ISBN. |
…ook generation and enhance metadata parsing in the API.