Backend passthrough service #63
Conversation
Troubleshoot/work in progress
Summary of ChangesHello @colin-roy-ehri, 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 introduces a significant architectural overhaul, transitioning the Looker Dashboard Summarization extension from a frontend-centric AI processing model to a robust backend service architecture. The primary goal is to streamline user experience by removing direct user OAuth for AI, centralizing authentication, and enhancing security. The changes involve implementing a new Cloud Run-based Node.js backend, refactoring the frontend to interact with this service, and updating the application's configuration and documentation to support this new paradigm. Highlights
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 a significant and beneficial architectural refactoring, moving from a direct frontend-to-Vertex AI model to a backend passthrough service. This simplifies the frontend, centralizes logic, and improves security by not exposing credentials to the client. The addition of detailed documentation in MIGRATION_NOTES.md and the updated README.md is commendable. However, the current implementation contains several critical issues, primarily related to hardcoded project IDs and other configuration values in deployment scripts and source code. There are also some outdated/broken files and inconsistencies in environment variable naming that need to be addressed. Once these issues are resolved, this will be a strong improvement to the application.
I am having trouble creating individual review comments. Click here to see my feedback.
restful-service/src/cloudbuild.yaml (4-6)
The Docker image path is hardcoded with a specific project ID (combined-genai-bi). This prevents the Cloud Build configuration from being reusable across different projects. You should use Cloud Build substitutions (e.g., $_PROJECT_ID or $PROJECT_ID) to make this dynamic.
args: ['build', '-t',
'us-central1-docker.pkg.dev/${PROJECT_ID}/dashboard-summarization-docker-repo/restfulserviceimage',
'.']src/utils/loadUserSettings.ts (1-73)
This utility appears to be outdated and does not align with the new architecture. It attempts to load old Vertex AI settings (vertexProject, googleOAuthClientId, etc.) from user attributes. However, the new architecture only requires backendServiceUrl.
The SettingsContext.tsx uses this file as a fallback, but the logic will fail because the VertexSettings type it returns doesn't match what the context expects. This breaks the settings migration path. This file should either be updated to load the backend_service_url attribute or be removed entirely if the user attribute fallback is no longer desired.
src/utils/generateArbitraryResponse.ts (2)
This line imports getCachedAIResponse and cacheAIResponse from caching.ts, but these functions are not defined in that file, which will cause a compilation error. Since these functions are not used in this file, this import should be removed.
restful-service/deploy.sh (9)
The PROJECT_ID is hardcoded in this deployment script. This makes the script non-portable and prone to errors if another developer tries to use it. Instead of hardcoding the value, it should be passed as an argument to the script or read from the active gcloud configuration.
PROJECT_ID=$(gcloud config get-value project)
restful-service/terraform/variables.tf (1-14)
The variables project_id and docker_image have hardcoded default values that are specific to a particular project. This makes the Terraform configuration difficult to reuse. For required variables like these, it's better to remove the default value, which will force the user to provide a value at runtime.
variable "project_id" {
type = string
description = "The GCP project ID to deploy resources into."
}
variable "deployment_region" {
type = string
default = "us-central1"
}
variable "docker_image" {
type = string
description = "The full path to the Docker image in Artifact Registry."
}
.gitignore (8-9)
The build output directory dist/ has been removed from the .gitignore file. Build artifacts should not be checked into version control. Please re-add this to the ignore list.
package.json (22-23)
This change downgrades React from version 18 to 17. This is a significant change that could impact performance and future development, as it removes access to concurrent features introduced in React 18. Was this downgrade intentional, for example, to address a compatibility issue with a dependency or the Looker extension framework? If so, it would be beneficial to document the reason for this change.
README.md (411)
The previous version of the README.md included a helpful section titled "Recommendations for fine tuning the model", which explained how to use dashboard details and tile notes to provide more context to the LLM. This information is still highly relevant with the new architecture and would be very valuable for users. Consider adding this section back to the documentation.
restful-service/src/index.js (75-76)
The VertexAI constructor includes hardcoded fallback values for project and location. If these environment variables are not set, the service will either fail unexpectedly or connect to the wrong project. It's better to enforce that these variables are explicitly configured by removing the fallbacks and throwing an error at startup if they are missing.
project: process.env.PROJECT,
location: process.env.REGION
restful-service/src/.env.example (1)
The PROJECT variable is hardcoded to a specific project ID. In an example file, this should be a placeholder to instruct the user to add their own project ID.
PROJECT=YOUR_GCP_PROJECT_ID
src/components/DashboardSummarization.tsx (113)
Checking for admin privileges by matching the role name admin is brittle, as role names can be changed. A more robust approach would be to check if the user has a specific, high-level permission that is typically reserved for admins, such as see_users or administer.
.env.example (1-8)
The environment variables in this example file are inconsistent with the documentation (README.md, MIGRATION_NOTES.md) and the application code (src/contexts/SettingsContext.tsx).
RESTFUL_SERVICEshould beBACKEND_SERVICE_URLto match its usage in the code.AI_CF_AUTH_TOKENappears to be unused. The backend authentication is handled by a secret passed as a header, configured via a Looker User Attribute, not an environment variable in the frontend.
To avoid confusion, this file should be updated to reflect the actual variables used.
# SLACK_CLIENT_ID=YOUR_SLACK_CLIENT_ID
# SLACK_CLIENT_SECRET=YOUR_SLACK_CLIENT_SECRET
# GOOGLE_CLIENT_ID=YOUR_GOOGLE_CLIENT_ID
# CHANNEL_ID=YOUR_SLACK_CHANNEL_ID
# SPACE_ID=YOUR_GOOGLE_SPACE_ID
BACKEND_SERVICE_URL=YOUR_CLOUD_RUN_URL
CLAUDE.md (49)
The documentation mentions RESTFUL_SERVICE as an environment variable. This is inconsistent with the README.md and the application code, which use BACKEND_SERVICE_URL. To ensure consistency across all documentation, please update this to BACKEND_SERVICE_URL.
- **.env**: Environment variables (SLACK_CLIENT_ID, BACKEND_SERVICE_URL, etc.)
src/utils/useAutoOAuth.ts (1-196)
This file, along with oauth-callback.tsx, implements the client-side OAuth flow that was part of the old architecture. According to MIGRATION_NOTES.md, this flow has been removed. These files appear to be dead code and should be deleted to clean up the codebase and avoid confusion.
1 participant
No description provided.