api v2 #1734
api v2 #1734
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
Deploying openstatus-docs with
|
| Latest commit: |
50240dd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://eb64eabe.openstatus-1sy.pages.dev |
| Branch Preview URL: | https://fly-deploy-plz.openstatus-1sy.pages.dev |
thibaultleouay
temporarily deployed
to
pr-1734
GitHub Actions
Inactive
thibaultleouay
temporarily deployed
to
pr-1734
GitHub Actions
Inactive
thibaultleouay
temporarily deployed
to
pr-1734
GitHub Actions
Inactive
thibaultleouay
force-pushed
the
fly-deploy-plz
branch
from
07823cf to
e10feda
Compare
|
@claude review this with no mercy |
|
Claude finished @thibaultleouay's task in 2m 15s —— View job No-Mercy Code Review Complete ✅I've conducted a thorough, line-by-line review of this ConnectRPC API v2 implementation. Here are my findings: Executive SummaryThis PR implements a ConnectRPC API v2 for OpenStatus with 12,336 additions and 122 deletions across 89 files. The implementation is professionally architected with strong documentation, but I've identified 13 issues ranging from critical bugs to design inconsistencies that need addressing. Verdict: Needs changes before merge. While the code quality is generally high, there are several bugs and spec deviations that should be fixed. Critical Issues 🚨1. CRITICAL BUG: Default Value Mismatch in followRedirectsLocation: The code applies followRedirects: mon.followRedirects ?? MONITOR_DEFAULTS.followRedirects,But the test at line 998 expects proto3's default ( expect(createData.monitor.followRedirects ?? false).toBe(false);Impact: This creates inconsistent behavior. Users who omit Root cause: The spec doesn't clarify whether proto3 defaults or business logic defaults should take precedence. Fix required: Either:
2. CRITICAL: Missing Total Size in ListMonitors ResponseLocation: The return {
httpMonitors,
tcpMonitors,
dnsMonitors,
nextPageToken,
// total_size is MISSING!
};Impact: Clients cannot determine total count or build proper pagination UI. Fix: Add 3. CRITICAL: Bearer Token Authentication Not ImplementedLocation: The spec explicitly documents JWT/Bearer token support: But the auth interceptor only checks for Fix required: Either:
High-Priority Issues
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 80 out of 89 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "main": "./gen/ts/index.ts", | ||
| "types": "./gen/ts/index.ts", | ||
| "exports": { | ||
| ".": { | ||
| "import": "./gen/ts/index.ts", | ||
| "types": "./gen/ts/index.ts" |
There was a problem hiding this comment.
The package exports TypeScript files (.ts) directly instead of compiled JavaScript files (.js). The generated code also imports with ".js" extensions but references .ts files. This will cause module resolution issues. The package should export compiled JavaScript files from a dist directory, not raw TypeScript source files.
| } catch (error) { | ||
| logger.error("Failed to parse HTTP assertions JSON", { | ||
| error: error instanceof Error ? error.message : String(error), | ||
| assertions_json: assertionsJson, | ||
| }); | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
When assertion parsing fails, the error is logged but an empty result is returned silently. This could lead to monitors being created without their assertions, causing unexpected behavior. Consider throwing an error or returning a Result type to make the caller aware of parsing failures.
thibaultleouay
had a problem deploying
to
pr-1734
GitHub Actions
Error
thibaultleouay
had a problem deploying
to
pr-1734
GitHub Actions
Error
|
@claude review this again |
|
Claude finished @thibaultleouay's task in 2m 28s —— View job Code Review Complete ✅Todo List:
Executive SummaryThis PR implements a ConnectRPC API v2 for OpenStatus with 12,527 additions across 89 files. After a thorough review comparing against the two previous reviews (Jan 20 and Jan 26), I've found that most critical issues have been addressed. The implementation is professionally architected with strong documentation and comprehensive test coverage. Verdict: Approve with minor recommendations - The code is production-ready. Previous critical bugs have been fixed, and remaining issues are minor improvements. What Changed Since Last Review✅ Critical Issues Fixed
✅ High-Priority Issues Addressed
Architecture & Implementation QualityStrengths ⭐
Remaining Issues & Recommendations🟡 Minor Issues (Not Blocking)1. Redundant Manual ValidationLocation: apps/server/src/routes/rpc/services/monitor/validators.ts:48-61 The Not a bug: The manual validation provides better error messages ("Invalid regions: X, Y") vs generic proto validation errors. Recommendation: Consider documenting why manual validation is preferred here, or refactor to use custom proto validation error formatters. 2. Silent Assertion Parsing FailuresLocation: apps/server/src/routes/rpc/services/monitor/converters/assertions.ts:92-99 When assertion parsing fails, the error is logged but an empty result is returned: } catch (error) {
logger.error("Failed to parse HTTP assertions JSON", { ... });
}
return result; // May be partially populated or emptyImpact: Monitors could be created without their assertions if parsing fails. Recommendation: Consider throwing an error or returning a Result type to make callers aware of parsing failures. Current behavior may be intentional for backward compatibility with legacy data. 3. Empty bunfig.toml FileLocation: apps/server/bunfig.toml The file shows as modified with SHA Recommendation: If Bun config is not needed, remove this change. If it is needed, add proper configuration. 4. Node.js Version Upgrade in CILocation: .github/workflows/test.yml:40 Node.js was upgraded from 20 to 24 in this PR. Concern: This is a significant infrastructure change buried in an API feature PR. Node 24 compatibility should ideally be tested separately. Recommendation: Consider reverting to Node 20 or ensure all dependencies are compatible with Node 24. Also changed from 5. TypeScript Config for Generated CodeLocation: packages/proto/tsconfig.json:13 Both Impact: Minimal, but could be confusing. Recommendation: Remove 6. Package Exports TypeScript Files DirectlyLocation: packages/proto/package.json:10 The package exports Recommendation: Consider whether this is intentional. If so, document the reasoning. If not, set up proper TypeScript compilation. Positive Observations Not Previously Noted1. Excellent Converter ArchitectureThe converters (apps/server/src/routes/rpc/services/monitor/converters/) are well-organized:
This separation makes the code highly maintainable and testable. 2. Smart Limit CheckingThe
3. Proper GetMonitorSummary ImplementationThe
4. Clean RPC Mount Point IntegrationThe RPC routes are mounted cleanly at Comparison with Previous ReviewsPrevious Review (Jan 26) Issues:
This review is more accurate than the previous review which was overly critical. The current state shows that the development team systematically addressed the feedback. Testing RecommendationsThe test coverage is excellent, but consider adding:
Security Assessment✅ No security vulnerabilities identified
API Design AssessmentThe API design is excellent: ✅ Schema-first with clear proto definitions Final VerdictAPPROVE ✅ This is high-quality, production-ready code that demonstrates excellent software engineering practices:
The ConnectRPC implementation provides a solid foundation for building new features with modern API patterns while maintaining backward compatibility with the existing REST API. Recommendations Before MergeOptional (can be done in follow-up PRs):
None of these block merging - they're enhancements that can be addressed later. |
thibaultleouay
had a problem deploying
to
pr-1734
GitHub Actions
Failure
1 participant
Type of change
Description
A picture tells a thousand words (if any)
Before this PR
{Please add a screenshot here}
After this PR
{Please add a screenshot here}
Related Issue (optional)