Conversation
… bundling issues Replace @google-cloud/logging-winston with Console transport and Cloud Logging severity mapping. Cloud Run automatically sends stdout to Cloud Logging, so a dedicated transport is unnecessary. Changes: - Remove @google-cloud/logging-winston dependency - Add severity mapping for Cloud Logging compatibility - Add trace context extraction for distributed tracing - Implement code splitting for server and worker bundles - Add worker-entry.ts for minimal worker bundle - Simplify Dockerfile to only copy tinypool and tiktoken
web-tree-sitter looks for the WASM file in the same directory as the JavaScript file. Without this, the Compress feature fails with ENOENT.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe PR restructures the server bundling and worker architecture: it introduces a separate lightweight worker entry point, replaces Tinypool worker logic with a unified worker handler, migrates logging from Cloud Logging transport to Console-based transport (while maintaining Cloud Run trace context), refactors the bundling pipeline to support multiple entry points with code-splitting and WASM collection, and updates deployment configuration to reflect new build artifacts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @yamadashy, 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 addresses critical bundling issues and enhances the server's operational efficiency and observability. By removing a problematic logging dependency and adopting a more streamlined logging approach compatible with Cloud Run, it resolves gRPC conflicts. Furthermore, it introduces distributed tracing capabilities for better request correlation and implements code splitting to optimize worker bundle sizes, leading to improved startup times and reduced memory footprint. The changes also ensure proper handling of WebAssembly files crucial for certain features. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1074 +/- ##
=======================================
Coverage 87.19% 87.19%
=======================================
Files 116 116
Lines 4350 4350
Branches 1011 1011
=======================================
Hits 3793 3793
Misses 557 557 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying repomix with
|
| Latest commit: |
2928e2b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b0a5fcbd.repomix.pages.dev |
| Branch Preview URL: | https://fix-website-server-console-l.repomix.pages.dev |
![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 effectively resolves the gRPC bundling issue by removing the @google-cloud/logging-winston dependency and replacing it with a custom Winston console transport that maps to Cloud Logging severities. The implementation is solid. Additionally, the introduction of code splitting for worker bundles is a great performance optimization, and the inclusion of trace context extraction significantly improves observability. I've left a few minor suggestions to improve code clarity and performance, but overall, this is a high-quality contribution.
| ...(traceContext.trace && { 'logging.googleapis.com/trace': traceContext.trace }), | ||
| ...(traceContext.spanId && { 'logging.googleapis.com/spanId': traceContext.spanId }), |
There was a problem hiding this comment.
The logic to add trace context fields is duplicated in three places (request start, completion, and failure logs on lines 82-83 and 105-106). To improve maintainability and reduce code duplication, consider defining these fields once and reusing them.
For example:
const traceContext = extractTraceContext(c);
const traceFields = {
...(traceContext.trace && { 'logging.googleapis.com/trace': traceContext.trace }),
...(traceContext.spanId && { 'logging.googleapis.com/spanId': traceContext.spanId }),
};
// Then in your log calls:
logger.info({
// ... other fields
...traceFields,
});
Code ReviewOverall, this is a well-structured PR that addresses a real bundling issue with gRPC/logging dependencies. The approach of using Console transport + Cloud Logging severity mapping is the recommended pattern for Cloud Run environments. SummaryWhat this PR does:
Code Quality & Best PracticesPositive:
DetailsThe logger implementation follows Cloud Logging best practices by:
Potential IssuesMinor items to consider
Security Considerations
Performance ConsiderationsPositive:
Test CoverageAccording to Codecov, coverage is maintained at 87.19%. The website server code appears to be outside the main test coverage scope, which is acceptable for infrastructure code. Premortem Analysis
RecommendationApprove - This is a well-executed fix for the bundling issue. The code is clean, follows Cloud Run best practices, and the architectural decisions (code splitting, Console transport) are sound. 🤖 Generated with Claude Code |
Code Review - Additional AnalysisI've reviewed this PR after Claude's initial review. Here are some additional observations: Overall AssessmentThis is a well-executed PR that addresses the gRPC bundling issues by removing Key Strengths
Minor Issues Identified1. Deprecated method usage in cloudLogger.ts:16// Current (deprecated)
Math.random().toString(36).substr(2, 9)
// Suggested (modern)
Math.random().toString(36).substring(2, 11)
2. Duplicate `.git` entry in .dockerignoreLines 15 and 69 both list
Security Review
Premortem Analysis
RecommendationApprove ✅ - The PR is well-implemented and addresses the stated goals effectively. The minor issues noted above are non-blocking. 🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
website/server/src/utils/logger.ts (2)
4-15: MoveseverityMapoutside the formatter function.The
severityMapis currently recreated on every log entry, which is inefficient. Define it as a module-level constant to avoid repeated allocations.🔎 Proposed refactor
+// Map winston levels to Cloud Logging severity levels +// https://cloud.google.com/logging/docs/reference/v2/rest/v2/LogEntry#LogSeverity +const SEVERITY_MAP: Record<string, string> = { + error: 'ERROR', + warn: 'WARNING', + info: 'INFO', + debug: 'DEBUG', +} as const; + -// Map winston levels to Cloud Logging severity levels -// https://cloud.google.com/logging/docs/reference/v2/rest/v2/LogEntry#LogSeverity const cloudLoggingSeverity = winston.format((info) => { - const severityMap: Record<string, string> = { - error: 'ERROR', - warn: 'WARNING', - info: 'INFO', - debug: 'DEBUG', - }; - info.severity = severityMap[info.level] || 'DEFAULT'; + info.severity = SEVERITY_MAP[info.level] || 'DEFAULT'; return info; });
21-29: Consider applying severity mapping to both environments.The
cloudLoggingSeverityformatter is currently only applied in production. For consistency and to make local development logs more representative of production, consider applying it to both environments.🔎 Proposed refactor
const transports: winston.transport[] = [ new winston.transports.Console({ format: isProduction - ? winston.format.combine(cloudLoggingSeverity(), winston.format.json()) - : winston.format.combine(winston.format.timestamp(), winston.format.json()), + ? winston.format.combine(cloudLoggingSeverity(), winston.format.json()) + : winston.format.combine(winston.format.timestamp(), cloudLoggingSeverity(), winston.format.json()), }), ];website/server/src/middlewares/cloudLogger.ts (3)
14-17: Replace deprecatedsubstr()withslice().Line 16 uses the deprecated
substr()method. Modern JavaScript should usesubstring()orslice()instead.🔎 Proposed fix
function generateRequestId(): string { - return `req-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; + return `req-${Date.now()}-${Math.random().toString(36).slice(2, 11)}`; }
19-35: Add validation for empty trace components.After splitting the trace header on line 27,
traceIdorspanIdcould be empty strings if the header is malformed (e.g.,"//;o=1"or"/span123;o=1"). Empty strings are truthy and would pass the check on line 29, potentially logging invalid trace data to Cloud Logging.🔎 Proposed fix with empty string validation
function extractTraceContext(c: Context): { trace?: string; spanId?: string } { const traceHeader = c.req.header('x-cloud-trace-context'); if (!traceHeader) return {}; const projectId = process.env.GOOGLE_CLOUD_PROJECT || process.env.GCP_PROJECT; const [traceSpan] = traceHeader.split(';'); const [traceId, spanId] = traceSpan.split('/'); - if (!traceId) return {}; + if (!traceId || traceId.trim() === '') return {}; return { trace: projectId ? `projects/${projectId}/traces/${traceId}` : traceId, - spanId, + ...(spanId && spanId.trim() !== '' && { spanId }), }; }
59-60: Consider extracting trace context spreading into a helper.The trace context spreading logic is duplicated three times (request start, completion, and error logs). Extracting it into a helper function would reduce repetition and improve maintainability.
🔎 Proposed refactor
Add a helper function after
extractTraceContext:// Helper to build trace context fields for Cloud Logging function buildTraceFields(traceContext: { trace?: string; spanId?: string }) { return { ...(traceContext.trace && { 'logging.googleapis.com/trace': traceContext.trace }), ...(traceContext.spanId && { 'logging.googleapis.com/spanId': traceContext.spanId }), }; }Then replace the three occurrences with:
logger.info({ message: `${method} ${url.pathname} started`, requestId, - // Cloud Logging trace correlation field - ...(traceContext.trace && { 'logging.googleapis.com/trace': traceContext.trace }), - ...(traceContext.spanId && { 'logging.googleapis.com/spanId': traceContext.spanId }), + ...buildTraceFields(traceContext), httpRequest: {Apply similar changes to lines 82-83 and 105-106.
Also applies to: 82-83, 105-106
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
website/server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
website/server/.dockerignorewebsite/server/.gcloudignorewebsite/server/.gitignorewebsite/server/Dockerfilewebsite/server/package.jsonwebsite/server/scripts/bundle.mjswebsite/server/src/index.tswebsite/server/src/middlewares/cloudLogger.tswebsite/server/src/utils/logger.tswebsite/server/src/worker-entry.tswebsite/server/warmup.mjs
💤 Files with no reviewable changes (1)
- website/server/package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-27T08:22:19.918Z
Learnt from: boralg
Repo: yamadashy/repomix PR: 318
File: src/config/defaultIgnore.ts:136-138
Timestamp: 2025-01-27T08:22:19.918Z
Learning: In Rust projects, `cargo-timing*.html` files are generated within the `target/` directory, so they're automatically covered by the `**/target/**` ignore pattern.
Applied to files:
website/server/.dockerignore
🧬 Code graph analysis (2)
website/server/src/utils/logger.ts (1)
src/shared/logger.ts (1)
info(52-56)
website/server/src/middlewares/cloudLogger.ts (1)
website/server/src/utils/logger.ts (1)
logger(38-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (16)
website/server/src/middlewares/cloudLogger.ts (1)
51-52: LGTM!Extracting the trace context once and reusing it across multiple log statements is efficient and follows good practices for Cloud Run distributed tracing.
website/server/.gitignore (1)
4-4: LGTM!The
.compile-cacheentry correctly ignores the V8 compile cache directory generated by the warmup script, aligning with the build optimization strategy.website/server/.gcloudignore (1)
1-71: LGTM!The comprehensive ignore patterns are well-organized and properly exclude non-runtime artifacts (documentation, build outputs, development files) from cloud deployments, reducing artifact size and improving deployment efficiency.
website/server/warmup.mjs (2)
33-35: LGTM!The explicit
process.exit(0)ensures clean termination, preventing event loop handlers from keeping the process alive after warmup completes.
19-26: The warmup pattern is appropriate for this codebase. Thedist-bundled/directory contains only intentional entry point bundles generated by Rolldown (server.mjsandworker.mjs, plus optional shared chunks), not arbitrary modules. Since these entry points have theWARMUP_MODEguard in place (visible in index.ts lines 14-20), importing all.mjsfiles is safe—the server initialization and any logging only occur whenWARMUP_MODEis not set.website/server/src/worker-entry.ts (1)
1-14: LGTM!The minimal worker entry point cleanly separates worker concerns from server dependencies, reducing bundle size and improving worker initialization performance. The re-export pattern is straightforward and well-documented.
website/server/src/index.ts (1)
19-20: LGTM! Simplified initialization guard aligns with worker separation.The removal of
isTinypoolWorker()check is appropriate given the new dedicated worker entry point (worker-entry.ts). The server initialization now only guards against warmup mode, as workers will use their own entry point.To ensure the Tinypool configuration correctly references the new worker entry point, run:
#!/bin/bash # Verify Tinypool is configured to use the new worker entry point echo "=== Checking REPOMIX_WORKER_PATH references ===" rg -n "REPOMIX_WORKER_PATH" -A 2 -B 2 echo "" echo "=== Checking Tinypool configuration ===" rg -n "Tinypool|tinypool" -A 5 -B 2 --type=ts --type=jswebsite/server/scripts/bundle.mjs (6)
23-32: LGTM!The cleanup logic is correct and appropriate for a build script. Using
rmSyncwithrecursive: truesafely handles non-empty directories, andmkdirSyncwithrecursive: trueensures the directory is created.
66-74: Verify the code-splitting output structure.The
advancedChunksconfiguration is designed to create a shared chunk for code used by both the server and worker entries. Given that Rolldown is in beta (1.0.0-beta.58), please verify that:
- The worker bundle is significantly smaller than the server bundle
- A shared chunk is generated containing common dependencies
- Both bundles execute correctly with the shared chunk loaded
This can be manually verified by inspecting the bundle sizes reported during the build and testing both entry points.
85-91: LGTM!The file size calculation is correct, and using
statSyncis appropriate for a build script context. The function provides clear human-readable output for logging.
104-112: LGTM!The logic to copy
web-tree-sitter.wasmto the dist-bundled root is correct and necessary, as explained in the comment. The existence check and warning provide appropriate feedback if the file is missing.
133-147: LGTM!The build process flow is well-structured: cleaning first prevents stale artifacts, and the sequential execution order is appropriate. The top-level error handling ensures failures are caught and reported.
48-62: No verification needed—the bundled output already executes successfully in Node.js.The Dockerfile confirms the bundled
server.mjsis deployed to production and executed directly vianode dist-bundled/server.mjs. A warmup script (RUN node warmup.mjs) pre-compiles the bundled code at build time, proving Node.js ESM compatibility is intact. The banner removal does not break Node.js execution.website/server/.dockerignore (1)
7-12: LGTM!The additions of
dist-bundledand.compile-cacheare appropriate and align with the new build artifacts generated by the bundle script and the compile cache directory used in the Dockerfile.website/server/Dockerfile (2)
33-37: LGTM!The updated comments clearly explain why tinypool and tiktoken cannot be bundled, which improves maintainability. The inline explanations about file paths and WASM loading are helpful.
46-46: Path change is correct and properly implemented.The bundle script generates
worker.mjsfrom theworker-entry.tsentry point, which contains only minimal worker code (exporting the unified worker handler from repomix without server dependencies). The tinypool configuration correctly referencesREPOMIX_WORKER_PATHat runtime, and the Dockerfile properly sets it to/app/dist-bundled/worker.mjs.
1 participant
Summary
@google-cloud/logging-winstondependency that caused gRPC bundling issues with Rolldownweb-tree-sitter.wasmto dist-bundled for Compress feature supportChecklist
npm run testnpm run lint