fix: add dataconnect-generated stubs and fix TypeScript errors in emb…#42
fix: add dataconnect-generated stubs and fix TypeScript errors in emb…#42groupthinking wants to merge 1 commit intomainfrom
Conversation
…eddings - Create stub types for Firebase Data Connect SDK - Add explicit type assertions for JSON responses - Add explicit VideoEmbedding type to map callbacks - Move dataconnect-generated inside src to satisfy rootDir constraint https://claude.ai/code/session_015Pd3a6hinTenCNrPRGiZqE
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
railway-app
bot
temporarily deployed
to
EventRelay / EventRelay-pr-42
|
🚅 Deployed to the EventRelay-pr-42 environment in EventRelay
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness and type safety of the embeddings package. It introduces mock implementations and type definitions for the Firebase Data Connect SDK, allowing the application to compile and run in environments lacking the auto-generated SDK. Additionally, it refines JSON response handling with explicit type assertions and clarifies data transformations with specific type annotations, leading to a more reliable and maintainable codebase. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces stub files for Firebase Data Connect to facilitate builds in environments where the SDK is unavailable, and it resolves several TypeScript errors by adding explicit type assertions. These changes enhance the type safety and robustness of the embeddings package, aligning with the project's strict type hinting standards. My review includes suggestions to further improve maintainability by using named interfaces for API responses to reduce code duplication and by exporting a shared UUID type for better reusability.
| } | ||
|
|
||
| // UUID type for job IDs | ||
| type UUID = `${string}-${string}-${string}-${string}-${string}`; |
There was a problem hiding this comment.
The UUID type is used in the signatures of exported functions like getJobEmbeddings, but it's not exported itself. Exporting it would allow other modules, like embedding.ts, to use it directly. This would avoid the need to redefine or cast to the template literal string `${string}-${string}-${string}-${string}-${string}`, reducing duplication and improving maintainability.
| type UUID = `${string}-${string}-${string}-${string}-${string}`; | |
| export type UUID = `${string}-${string}-${string}-${string}-${string}`; |
| const data = (await response.json()) as { | ||
| predictions: Array<{ embeddings: { values: number[] } }>; | ||
| }; |
There was a problem hiding this comment.
This anonymous type for the API response is also used in the generateEmbedding function. To improve maintainability and avoid code duplication, it would be better to define a named interface (e.g., EmbeddingResponse) in a shared location (like the types section at the top of this file) and reuse it here and in generateEmbedding.
| { headers: { "Metadata-Flavor": "Google" } } | ||
| ); | ||
| const data = await response.json(); | ||
| const data = (await response.json()) as { access_token: string }; |
There was a problem hiding this comment.
To improve readability and maintainability, consider defining a named interface for this API response shape (e.g., AccessTokenResponse) instead of using an inline anonymous type. This makes the code more self-documenting and easier to manage.
There was a problem hiding this comment.
Pull request overview
Adds in-repo Firebase Data Connect SDK stubs and tightens TypeScript typing in the embeddings package to fix build/type errors, while keeping generated code under src/ to satisfy the package’s rootDir constraint.
Changes:
- Adds
src/dataconnect-generatedstub module (types + functions that throw) to unblock builds when Data Connect SDK isn’t present. - Adds explicit JSON response type assertions for Vertex AI embedding responses and metadata-server access token responses.
- Imports and applies an explicit
VideoEmbeddingtype for Data Connect query result mapping.
Reviewed changes
Copilot reviewed 3 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/embeddings/src/embedding.ts | Updates Data Connect import path, adds JSON response assertions, and types Data Connect mapping callbacks. |
| packages/embeddings/src/dataconnect-generated/index.ts | Introduces TypeScript stub interfaces + stubbed SDK functions that throw when called. |
| packages/embeddings/src/dataconnect-generated/index.js | Checked-in compiled stub output (JS). |
| packages/embeddings/src/dataconnect-generated/index.js.map | Checked-in source map for compiled stub output. |
| packages/embeddings/src/dataconnect-generated/index.d.ts | Checked-in generated declaration output. |
| packages/embeddings/src/dataconnect-generated/index.d.ts.map | Checked-in declaration source map. |
| @@ -148,7 +153,7 @@ async function getAccessToken(): Promise<string> { | |||
| "http://metadata.google.internal/computeMetadata/v1/instance/service-accounts/default/token", | |||
| { headers: { "Metadata-Flavor": "Google" } } | |||
| ); | |||
There was a problem hiding this comment.
In getAccessToken(), the metadata server fetch result isn’t checked before calling response.json(). If the metadata server returns a non-2xx (e.g., permission/availability issues), this will throw a less-informative JSON parsing error. Consider checking response.ok and throwing an error that includes the status and response body before parsing.
| ); | |
| ); | |
| if (!response.ok) { | |
| const body = await response.text().catch(() => "<unable to read response body>"); | |
| throw new Error(`Metadata server error: ${response.status} ${body}`); | |
| } |
| /** | ||
| * Stub types for Firebase Data Connect SDK | ||
| * | ||
| * In production, this file would be auto-generated by Firebase Data Connect. | ||
| * For Vercel builds where Data Connect isn't available, we provide stubs | ||
| * that throw helpful errors at runtime. | ||
| */ |
There was a problem hiding this comment.
These dataconnect-generated stubs are TypeScript source, but this PR also checks in compiled artifacts (index.js, .map, .d.ts, .d.ts.map) under src/. Keeping build outputs alongside sources is prone to drift and can confuse module resolution; it’s usually better to commit only the .ts stub here and let tsc generate JS/declarations into dist/ during the package build.
| "use strict"; | ||
| /** | ||
| * Stub types for Firebase Data Connect SDK | ||
| * | ||
| * In production, this file would be auto-generated by Firebase Data Connect. | ||
| * For Vercel builds where Data Connect isn't available, we provide stubs | ||
| * that throw helpful errors at runtime. | ||
| */ | ||
| Object.defineProperty(exports, "__esModule", { value: true }); |
There was a problem hiding this comment.
This appears to be a compiled build artifact checked into src/ (along with source maps). Since the package build already runs tsc to produce dist/, keeping this file in src/ risks stale code and duplicate/ambiguous module resolution. Consider removing it from source control and generating it in dist/ instead.
|
Closing — these fixes were already merged via PR #41 (fix/embeddings-build-errors). This branch is stale. |
…eddings
https://claude.ai/code/session_015Pd3a6hinTenCNrPRGiZqE