Detailed Page Information #47
Conversation
|
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. WalkthroughMigrates API from cursor-based to page-based pagination (adds PageInfo), updates protobuf package/options, removes several language-specific gRPC SDK wrappers and devcontainer, restructures TS SDK and build tooling, adjusts buf/Makefile generation outputs, updates CI npm steps, and updates various gitignore/build metadata. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as Octopus API (v1)
participant Store as Database / Backend
Client->>API: GET /v1/entries?paginator.page={n}&paginator.pageSize={s}
note right of API#lightblue: API reads paginator.page (1-based)\nand page_size
API->>Store: Query entries OFFSET (page-1)*page_size LIMIT page_size
Store-->>API: entries + total_items
API->>API: compute page_info (current_page, current_page_size,\ntotal_pages, next_page, prev_page)
API-->>Client: 200 OK { items: [...], pageInfo: {currentPage, currentPageSize,...} }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
proto/api/v1/api.proto (1)
134-143: Breaking changes and outdated documentation in Paginator.
Breaking wire change: The summary indicates
page_sizewas moved from tag 1 to tag 2, andcursor(tag 2) was removed. Renumbering and removing tags breaks wire compatibility with existing clients.Stale comment: Line 135 still references "cursor-pagination" but implementation is now page-based. Update the comment to reflect the new pagination model.
🔎 Suggested documentation fix:
/** - * Paginator for database entry cursor-pagination. + * Paginator for database entry page-based pagination. */ message Paginator {sdk/openapi/octopus.swagger.json (1)
332-347: Outdated description references cursor-pagination.Line 346 still describes this as "Paginator for database entry cursor-pagination" but the implementation is now page-based. This should be corrected in the proto source and the swagger regenerated.
🔎 If manually editing, apply this fix:
- "description": "*\nPaginator for database entry cursor-pagination." + "description": "*\nPaginator for database entry page-based pagination."
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
sdk/golang/gen/api/v1/api.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (2)
proto/api/v1/api.proto(3 hunks)sdk/openapi/octopus.swagger.json(3 hunks)
⏰ 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: Agent
🔇 Additional comments (4)
proto/api/v1/api.proto (2)
145-154: PageInfo structure looks well-designed.The new
PageInfomessage provides comprehensive pagination metadata withnext_pageandprev_pageas optional fields for convenient navigation. This aligns well with the PR objective to make pagination more developer-friendly.
85-88: No breaking wire-format changes detected in the Object message field tags.The current state shows
expired_at = 3anddeleted_at = 4. Git history contains only one commit for this proto file ("Generate protobufs") with no prior versions available to verify tag renumbering. Changing field numbers is not safe and is equivalent to deleting and adding fields, but no evidence in the repository supports that this renumbering actually occurred. If these are the original field tag assignments, no backward compatibility issue exists.Likely an incorrect or invalid review comment.
sdk/openapi/octopus.swagger.json (2)
76-90: Pagination parameters correctly updated.The query parameters now use
paginator.pageandpaginator.pageSizewith proper integer types, aligning with the proto changes for page-based pagination.
297-331: v1PageInfo structure is comprehensive.The structure provides all necessary pagination metadata. Note that
totalItemsuses"type": "string"with"format": "int64"which is the correct OpenAPI 2.0 pattern for representing 64-bit integers in JSON (avoids JavaScript precision loss).
| "pageInfo": { | ||
| "$ref": "#/definitions/v1PageInfo", | ||
| "description": "Page information's about the cursor." | ||
| } |
There was a problem hiding this comment.
Same typo as proto file.
The description contains a typo: "Page information's about the cursor" should be "Page information about the response". Since this swagger file appears to be generated from the proto, fix the source proto comment and regenerate.
🤖 Prompt for AI Agents
In sdk/openapi/octopus.swagger.json around lines 251 to 254, the description for
"pageInfo" contains a typo and incorrect phrasing ("Page information's about the
cursor"); update the source proto comment to read "Page information about the
response" (or equivalent correct wording), then regenerate the swagger JSON from
the proto so the corrected description is reflected in this file.
There was a problem hiding this comment.
Pull request overview
This pull request refactors the pagination mechanism from cursor-based to page-based pagination and adds comprehensive page information to API responses. The changes provide clients with detailed pagination metadata including current page, total pages, and total items.
- Replaces cursor-based pagination with 1-based page index pagination
- Introduces a new
PageInfomessage with detailed pagination metadata (total pages, total items, next/prev page pointers) - Corrects protobuf field numbering in the
Objectmessage (expired_at and deleted_at)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| proto/api/v1/api.proto | Updates Paginator to use page-based pagination, adds new PageInfo message, updates GetResponse to return PageInfo instead of next_cursor, and corrects Object field numbers |
| sdk/openapi/octopus.swagger.json | Synchronizes OpenAPI schema with proto changes, updates parameter definitions and response schemas |
| sdk/golang/gen/api/v1/api.pb.go | Regenerated Go code reflecting the proto changes, including new PageInfo struct and updated Paginator |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sdk/go/v1/api.pb.go
Outdated
| NextPage *int32 `protobuf:"varint,5,opt,name=next_page,json=nextPage,proto3,oneof" json:"next_page,omitempty"` // next page id. null if not present | ||
| PrevPage *int32 `protobuf:"varint,6,opt,name=prev_page,json=prevPage,proto3,oneof" json:"prev_page,omitempty"` // previous page id. null if not present |
There was a problem hiding this comment.
The comment "next page id. null if not present" is misleading. The field is named "NextPage" and represents a page number (1-based index), not an "id". This should be clarified as "next page number" to be consistent with the field naming and the Page field description.
| NextPage *int32 `protobuf:"varint,5,opt,name=next_page,json=nextPage,proto3,oneof" json:"next_page,omitempty"` // next page id. null if not present | |
| PrevPage *int32 `protobuf:"varint,6,opt,name=prev_page,json=prevPage,proto3,oneof" json:"prev_page,omitempty"` // previous page id. null if not present | |
| NextPage *int32 `protobuf:"varint,5,opt,name=next_page,json=nextPage,proto3,oneof" json:"next_page,omitempty"` // next page number. null if not present | |
| PrevPage *int32 `protobuf:"varint,6,opt,name=prev_page,json=prevPage,proto3,oneof" json:"prev_page,omitempty"` // previous page number. null if not present |
sdk/openapi/octopus.swagger.json
Outdated
| "description": "Cursor for next page. Empty means \"no more results\"." | ||
| "pageInfo": { | ||
| "$ref": "#/definitions/v1PageInfo", | ||
| "description": "Page information's about the cursor." |
There was a problem hiding this comment.
The possessive form "information's" is grammatically incorrect in this context. It should be "Page information about the cursor" (removing the apostrophe and 's').
| "description": "Page information's about the cursor." | |
| "description": "Page information about the cursor." |
sdk/go/v1/api.pb.go
Outdated
| @@ -331,15 +415,15 @@ type GetResponse struct { | |||
| state protoimpl.MessageState `protogen:"open.v1"` | |||
| // List of found entries including revision, ID and timestamps. | |||
| Entries []*Entry `protobuf:"bytes,1,rep,name=entries,proto3" json:"entries,omitempty"` | |||
| // Cursor for next page. Empty means "no more results". | |||
| NextCursor string `protobuf:"bytes,2,opt,name=next_cursor,json=nextCursor,proto3" json:"next_cursor,omitempty"` | |||
| // Page information's about the cursor. | |||
There was a problem hiding this comment.
The possessive form "information's" is grammatically incorrect in this context. It should be "Page information about the cursor" (removing the apostrophe and 's').
| // Page information's about the cursor. | |
| // Page information about the cursor. |
| "title": "last page number" | ||
| }, | ||
| "totalItems": { | ||
| "type": "string", |
There was a problem hiding this comment.
The type for "totalItems" is defined as "string" with format "int64", which is inconsistent. For integer values like total item counts, the type should be "integer" with format "int64", not "string". This mismatch between type and format could cause issues for API consumers and code generators.
| "type": "string", | |
| "type": "integer", |
sdk/go/v1/api.pb.go
Outdated
| NextPage *int32 `protobuf:"varint,5,opt,name=next_page,json=nextPage,proto3,oneof" json:"next_page,omitempty"` // next page id. null if not present | ||
| PrevPage *int32 `protobuf:"varint,6,opt,name=prev_page,json=prevPage,proto3,oneof" json:"prev_page,omitempty"` // previous page id. null if not present |
There was a problem hiding this comment.
The comment "previous page id. null if not present" is misleading. The field is named "PrevPage" and represents a page number (1-based index), not an "id". This should be clarified as "previous page number" to be consistent with the field naming and the Page field description.
| NextPage *int32 `protobuf:"varint,5,opt,name=next_page,json=nextPage,proto3,oneof" json:"next_page,omitempty"` // next page id. null if not present | |
| PrevPage *int32 `protobuf:"varint,6,opt,name=prev_page,json=prevPage,proto3,oneof" json:"prev_page,omitempty"` // previous page id. null if not present | |
| NextPage *int32 `protobuf:"varint,5,opt,name=next_page,json=nextPage,proto3,oneof" json:"next_page,omitempty"` // next page number. null if not present | |
| PrevPage *int32 `protobuf:"varint,6,opt,name=prev_page,json=prevPage,proto3,oneof" json:"prev_page,omitempty"` // previous page number. null if not present |
sdk/openapi/octopus.swagger.json
Outdated
| "title": "next page id. null if not present" | ||
| }, | ||
| "prevPage": { | ||
| "type": "integer", | ||
| "format": "int32", | ||
| "title": "previous page id. null if not present" |
There was a problem hiding this comment.
The description "next page id. null if not present" is misleading. The field is named "nextPage" and represents a page number (1-based index), not an "id". This should be clarified as "next page number" to be consistent with the field naming and the "page" field description.
| "title": "next page id. null if not present" | |
| }, | |
| "prevPage": { | |
| "type": "integer", | |
| "format": "int32", | |
| "title": "previous page id. null if not present" | |
| "title": "next page number. null if not present" | |
| }, | |
| "prevPage": { | |
| "type": "integer", | |
| "format": "int32", | |
| "title": "previous page number. null if not present" |
sdk/openapi/octopus.swagger.json
Outdated
| "title": "next page id. null if not present" | ||
| }, | ||
| "prevPage": { | ||
| "type": "integer", | ||
| "format": "int32", | ||
| "title": "previous page id. null if not present" |
There was a problem hiding this comment.
The description "previous page id. null if not present" is misleading. The field is named "prevPage" and represents a page number (1-based index), not an "id". This should be clarified as "previous page number" to be consistent with the field naming and the "page" field description.
| "title": "next page id. null if not present" | |
| }, | |
| "prevPage": { | |
| "type": "integer", | |
| "format": "int32", | |
| "title": "previous page id. null if not present" | |
| "title": "next page number. null if not present" | |
| }, | |
| "prevPage": { | |
| "type": "integer", | |
| "format": "int32", | |
| "title": "previous page number. null if not present" |
|
TBH I can't really tell much more than Copilot.. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
proto/v1/api.proto (2)
152-153: Address previous comment: Clarify "page number" terminology.Previous reviewers noted that "page id" is misleading since these fields represent page numbers (1-based indices), not IDs. The existing comments should be updated for clarity.
As per previous review by Copilot:
optional int32 next_page = 5; // next page number (1-based). null if not present optional int32 prev_page = 6; // previous page number (1-based). null if not present
163-164: Address previous comments: Fix typo and wire-format breaking change.Two issues previously flagged by reviewers:
- Typo: "Page information's about the cursor" should be "Page information about the response"
- Critical wire-format issue: Replacing
next_cursor(string) withpage_info(PageInfo message) at tag 2 is a breaking changeAs noted by coderabbitai[bot], if backward compatibility is needed, keep
next_cursorat tag 2 (mark deprecated) and addpage_infoas a new tag number.🔎 Suggested fix from previous review
- // Page information's about the cursor. + // Page information about the response. PageInfo page_info = 2;For wire compatibility, consider:
optional string next_cursor = 2 [deprecated = true]; // Deprecated: use page_info instead PageInfo page_info = 3;sdk/openapi/octopus.swagger.json (2)
251-254: Typo already flagged in previous reviews.The typo "Page information's about the cursor" was already identified. Fix this in the source proto and regenerate.
320-329: Terminology already flagged: "page id" should be "page number".The misleading "page id" terminology in nextPage/prevPage titles was already flagged in previous reviews. These should read "next page number" and "previous page number" to match the field semantics. Fix in source proto and regenerate.
🧹 Nitpick comments (4)
settings.gradle.kts (1)
1-1: Remove unused import from internal Gradle API.This import from Gradle's internal package is not used anywhere in the file and should be removed. Importing internal APIs is risky since they can change without notice in future Gradle versions and are not part of the public API contract.
🔎 Proposed fix
-import org.gradle.kotlin.dsl.internal.relocated.kotlin.metadata.internal.metadata.deserialization.VersionRequirementTable.Companion.create -Makefile (1)
1-2: Consider safer cleanup with find or explicit subdirectory paths.The cleanup now removes entire top-level SDK directories (
sdk/java,sdk/golang, etc.). If any non-generated source files are accidentally placed in these directories, they will be permanently deleted.🔎 Suggested safer cleanup approach
generate-proto:: - rm -rf ./sdk/java && rm -rf ./sdk/golang && rm -rf ./sdk/ts/src/gen && rm -rf ./sdk/openapi && buf generate --template buf.gen.yaml + rm -rf ./sdk/java/src/java && rm -rf ./sdk/golang/gen && rm -rf ./sdk/ts/src/gen && rm -rf ./sdk/openapi && buf generate --template buf.gen.yamlAlternatively, add a safety check to warn if unexpected files exist, or use
.gitignorepatterns to protect source files.buf.gen.yaml (1)
5-31: Consider pinning plugin versions for reproducible builds.The plugin remotes have been changed from versioned (e.g.,
protocolbuffers/java:v25.3) to generic remotes without version pins. This can lead to non-reproducible builds and potential breakage if plugin updates introduce breaking changes.🔎 Recommended version pinning
- - remote: buf.build/protocolbuffers/java + - remote: buf.build/protocolbuffers/java:v28.3 out: sdk/java/src/java - - remote: buf.build/grpc/java + - remote: buf.build/grpc/java:v1.70.0 out: sdk/java/src/java - - remote: buf.build/protocolbuffers/go + - remote: buf.build/protocolbuffers/go:v1.36.0 out: sdk/go opt: - paths=source_relative - - remote: buf.build/grpc/go + - remote: buf.build/grpc/go:v1.5.1 out: sdk/go opt: - paths=source_relative - - remote: buf.build/bufbuild/es + - remote: buf.build/bufbuild/es:v2.2.3 out: sdk/ts/src/gen opt: - target=ts include_imports: true - - remote: buf.build/grpc-ecosystem/openapiv2 + - remote: buf.build/grpc-ecosystem/openapiv2:v2.27.2 out: sdk/openapi opt: - allow_merge=true - merge_file_name=octopusNote: Replace versions with the latest stable versions you want to use.
sdk/java/src/java/studio/o7/octopus/sdk/v1/Object.java (1)
16-37: Consider the impact of naming this messageObjectin JavaUsing
Objectas the message name in this package means unqualifiedObjectreferences instudio.o7.octopus.sdk.v1resolve to this type, notjava.lang.Object. That’s legal but can be confusing for manual Java consumers; if you expect a lot of hand-written Java against this SDK and can still change the proto safely, consider a less generic name (e.g.,OctopusObject) and regenerate.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sumsdk/go/v1/api.pb.gois excluded by!**/*.pb.gosdk/go/v1/api_grpc.pb.gois excluded by!**/*.pb.gosdk/golang/gen/api/v1/api.pb.gois excluded by!**/*.pb.go,!**/gen/**sdk/js/package-lock.jsonis excluded by!**/package-lock.jsonsdk/ts/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (41)
.devcontainer/devcontainer.json.github/workflows/ci.ymlMakefileREADME.mdbuf.gen.yamlbuf.yamlgo.modproto/v1/api.protosdk/golang/octopus/root.gosdk/java/.gitignoresdk/java/build.gradle.ktssdk/java/src/java/studio/o7/octopus/sdk/v1/Api.javasdk/java/src/java/studio/o7/octopus/sdk/v1/Entry.javasdk/java/src/java/studio/o7/octopus/sdk/v1/EntryOrBuilder.javasdk/java/src/java/studio/o7/octopus/sdk/v1/EventCall.javasdk/java/src/java/studio/o7/octopus/sdk/v1/EventCallOrBuilder.javasdk/java/src/java/studio/o7/octopus/sdk/v1/GetRequest.javasdk/java/src/java/studio/o7/octopus/sdk/v1/GetRequestOrBuilder.javasdk/java/src/java/studio/o7/octopus/sdk/v1/GetResponse.javasdk/java/src/java/studio/o7/octopus/sdk/v1/GetResponseOrBuilder.javasdk/java/src/java/studio/o7/octopus/sdk/v1/ListenMessage.javasdk/java/src/java/studio/o7/octopus/sdk/v1/ListenMessageOrBuilder.javasdk/java/src/java/studio/o7/octopus/sdk/v1/ListenRegister.javasdk/java/src/java/studio/o7/octopus/sdk/v1/ListenRegisterOrBuilder.javasdk/java/src/java/studio/o7/octopus/sdk/v1/Object.javasdk/java/src/java/studio/o7/octopus/sdk/v1/ObjectOrBuilder.javasdk/java/src/java/studio/o7/octopus/sdk/v1/OctopusGrpc.javasdk/java/src/java/studio/o7/octopus/sdk/v1/PageInfo.javasdk/java/src/java/studio/o7/octopus/sdk/v1/PageInfoOrBuilder.javasdk/java/src/java/studio/o7/octopus/sdk/v1/Paginator.javasdk/java/src/java/studio/o7/octopus/sdk/v1/PaginatorOrBuilder.javasdk/java/src/main/java/studio/o7/octopus/sdk/OctopusSDK.javasdk/java/src/main/java/studio/o7/octopus/sdk/structs/ProtoStruct.javasdk/js/.gitignoresdk/openapi/octopus.swagger.jsonsdk/ts/.gitignoresdk/ts/package.jsonsdk/ts/src/index.tssdk/ts/tsconfig.build.jsonsdk/ts/tsconfig.jsonsettings.gradle.kts
💤 Files with no reviewable changes (7)
- sdk/golang/octopus/root.go
- sdk/java/.gitignore
- sdk/js/.gitignore
- sdk/java/src/main/java/studio/o7/octopus/sdk/OctopusSDK.java
- .devcontainer/devcontainer.json
- sdk/java/src/main/java/studio/o7/octopus/sdk/structs/ProtoStruct.java
- sdk/java/build.gradle.kts
✅ Files skipped from review due to trivial changes (2)
- sdk/ts/.gitignore
- buf.yaml
🧰 Additional context used
🧬 Code graph analysis (10)
sdk/java/src/java/studio/o7/octopus/sdk/v1/ListenRegisterOrBuilder.java (1)
sdk/java/src/java/studio/o7/octopus/sdk/v1/ListenRegister.java (1)
com(17-644)
sdk/java/src/java/studio/o7/octopus/sdk/v1/EntryOrBuilder.java (2)
sdk/java/src/java/studio/o7/octopus/sdk/v1/Entry.java (1)
com(16-1096)sdk/java/src/java/studio/o7/octopus/sdk/v1/Object.java (1)
com(16-1246)
sdk/java/src/java/studio/o7/octopus/sdk/v1/OctopusGrpc.java (1)
sdk/go/v1/api.pb.go (15)
GetRequest(178-198)GetRequest(211-211)GetRequest(226-228)GetResponse(414-422)GetResponse(435-435)GetResponse(450-452)Entry(104-116)Entry(129-129)Entry(144-146)ListenMessage(470-479)ListenMessage(492-492)ListenMessage(507-509)EventCall(554-562)EventCall(575-575)EventCall(590-592)
sdk/java/src/java/studio/o7/octopus/sdk/v1/Object.java (1)
sdk/java/src/java/studio/o7/octopus/sdk/v1/Api.java (1)
com(8-214)
sdk/java/src/java/studio/o7/octopus/sdk/v1/EventCall.java (4)
sdk/java/src/java/studio/o7/octopus/sdk/v1/Api.java (1)
com(8-214)sdk/java/src/java/studio/o7/octopus/sdk/v1/ListenMessage.java (2)
com(16-866)Builder(348-818)sdk/java/src/java/studio/o7/octopus/sdk/v1/Object.java (2)
com(16-1246)Builder(426-1198)sdk/go/v1/api.pb.go (3)
EventCall(554-562)EventCall(575-575)EventCall(590-592)
sdk/java/src/java/studio/o7/octopus/sdk/v1/PageInfo.java (1)
sdk/go/v1/api.pb.go (3)
PageInfo(328-338)PageInfo(351-351)PageInfo(366-368)
sdk/java/src/java/studio/o7/octopus/sdk/v1/ListenMessage.java (2)
sdk/java/src/java/studio/o7/octopus/sdk/v1/Api.java (1)
com(8-214)sdk/java/src/java/studio/o7/octopus/sdk/v1/ListenRegister.java (2)
com(17-644)Builder(297-596)
sdk/java/src/java/studio/o7/octopus/sdk/v1/Api.java (7)
sdk/java/src/java/studio/o7/octopus/sdk/v1/EventCall.java (1)
com(16-780)sdk/java/src/java/studio/o7/octopus/sdk/v1/GetRequest.java (1)
com(16-1573)sdk/java/src/java/studio/o7/octopus/sdk/v1/ListenMessage.java (1)
com(16-866)sdk/java/src/java/studio/o7/octopus/sdk/v1/ListenRegister.java (1)
com(17-644)sdk/java/src/java/studio/o7/octopus/sdk/v1/Object.java (1)
com(16-1246)sdk/java/src/java/studio/o7/octopus/sdk/v1/PageInfo.java (1)
com(11-921)sdk/java/src/java/studio/o7/octopus/sdk/v1/Paginator.java (1)
com(16-539)
sdk/java/src/java/studio/o7/octopus/sdk/v1/ListenRegister.java (2)
sdk/java/src/java/studio/o7/octopus/sdk/v1/Api.java (1)
com(8-214)sdk/go/v1/api.pb.go (3)
ListenRegister(611-623)ListenRegister(636-636)ListenRegister(651-653)
sdk/java/src/java/studio/o7/octopus/sdk/v1/GetRequest.java (2)
sdk/java/src/java/studio/o7/octopus/sdk/v1/Api.java (1)
com(8-214)sdk/go/v1/api.pb.go (6)
GetRequest(178-198)GetRequest(211-211)GetRequest(226-228)Paginator(274-282)Paginator(295-295)Paginator(310-312)
🔇 Additional comments (28)
.github/workflows/ci.yml (1)
121-135: LGTM! Correctly updated npm workflow paths.All npm-related steps have been properly updated to use the new
sdk/tsdirectory, aligning with the migration from JavaScript to TypeScript SDK structure.README.md (1)
1-47: LGTM! Well-structured documentation.The README has been significantly improved with clear sections for requirements, SDK generation, and language-specific installation instructions. This makes the repository much more accessible to developers.
sdk/java/src/java/studio/o7/octopus/sdk/v1/ListenRegisterOrBuilder.java (1)
1-48: Generated code looks consistent.This protobuf-generated interface correctly exposes accessors for the
ListenRegistermessage fields defined inproto/v1/api.proto.sdk/ts/package.json (1)
6-40: LGTM! Well-structured TypeScript package configuration.The package has been properly configured for a TypeScript-first approach:
- Build outputs consolidated to
dist/directory- Proper
exportsfield for modern module resolution@bufbuild/protobufcorrectly moved topeerDependenciesto avoid version conflictsThis follows best practices for TypeScript library packaging.
proto/v1/api.proto (1)
85-88: Field tag renumbering claim cannot be verified from current codebase state.The review claims
expired_atanddeleted_atfields were renumbered (4→3 and 5→4), but no evidence of previous field assignments exists in the repository. The git history shows only one commit to this file, and no CHANGELOG, migration documentation, or version indicators that would confirm this breaking change occurred. Manual verification is required to confirm whether the field tags were actually renumbered from different values or if this is a new protobuf definition.sdk/openapi/octopus.swagger.json (1)
315-318: Thetype: "string"withformat: "int64"is correct for protobuf.Note: The earlier Copilot suggestion to change this to
"type": "integer"is incorrect. Protobuf JSON encoding representsint64as a string to avoid JavaScript number precision loss (JS numbers are 64-bit floats). This is the expected behavior from protobuf code generators.sdk/ts/src/index.ts (1)
1-1: LGTM!Clean barrel export for the SDK entry point, re-exporting generated protobuf bindings.
sdk/ts/tsconfig.build.json (1)
1-11: LGTM!Standard TypeScript build configuration that correctly extends the base config and enables declaration output for the distributable package.
sdk/ts/tsconfig.json (1)
1-13: LGTM!Appropriate TypeScript configuration with modern ES2022 target, bundler-compatible module resolution, and strict type checking. The
noEmit: truebase setting correctly defers output to the build config.sdk/java/src/java/studio/o7/octopus/sdk/v1/PaginatorOrBuilder.java (1)
1-32: Generated protobuf code - no issues.This is auto-generated code from
v1/api.proto. The interface correctly exposesgetPage()andgetPageSize()accessors for the Paginator message.sdk/java/src/java/studio/o7/octopus/sdk/v1/PageInfoOrBuilder.java (1)
1-90: Generated protobuf code - documentation inherits proto source issues.This is auto-generated code from
v1/api.proto. The interface structure is correct, providing presence-aware accessors for optional fields (hasNextPage,hasPrevPage). The "page id" terminology in the Javadoc comments (lines 55, 64, 74, 84) will be corrected when the source proto is updated as flagged in the swagger review.sdk/java/src/java/studio/o7/octopus/sdk/v1/Paginator.java (1)
1-539: Generated protobuf code - structure is correct.This is auto-generated code from
v1/api.protowith standard protobuf message implementation patterns. The "cursor-pagination" reference in the class documentation (lines 10-11, 251-252) will be corrected when the source proto is updated as flagged in the swagger review.sdk/java/src/java/studio/o7/octopus/sdk/v1/EventCallOrBuilder.java (1)
1-59: Generated protobuf code - no issues.This is auto-generated code from
v1/api.proto. The interface correctly exposes accessors for theobjectfield (with presence check and OrBuilder variant) and thecall_idstring field (with bytes accessor).sdk/java/src/java/studio/o7/octopus/sdk/v1/ListenMessageOrBuilder.java (1)
1-44: Generated protobuf code looks correct.This is a standard protobuf-generated
OrBuilderinterface for theListenMessageoneof. The interface correctly exposes accessors for bothcallback(EventCall) andregister(ListenRegister) alternatives, along with thegetMsgCase()discriminator.sdk/java/src/java/studio/o7/octopus/sdk/v1/GetResponseOrBuilder.java (1)
1-83: Generated protobuf code correctly exposes pagination metadata.This
GetResponseOrBuilderinterface properly exposes:
- Repeated
entriesfield with standard list accessorspage_infofield with presence check and accessor methodsThe
PageInfointegration aligns with the PR objective of providing more developer-friendly pagination information.sdk/java/src/java/studio/o7/octopus/sdk/v1/EntryOrBuilder.java (1)
1-96: Generated protobuf interface is correct.The
EntryOrBuilderinterface properly exposes accessors for all Entry fields (object,created_at,revision,id) following standard protobuf patterns.sdk/java/src/java/studio/o7/octopus/sdk/v1/ObjectOrBuilder.java (1)
1-113: Generated protobuf interface is correct.The
ObjectOrBuilderinterface correctly exposes accessors for all Object fields, with appropriate presence checks (hasExpiredAt(),hasDeletedAt()) for the optional timestamp fields.sdk/java/src/java/studio/o7/octopus/sdk/v1/Api.java (2)
1-4: Verify: Should generated protobuf code be checked in?The file header includes
NO CHECKED-IN PROTOBUF GENCODE, which typically indicates generated files should not be committed to version control. However, some projects intentionally commit generated code for reproducibility or to avoid build-time protoc dependencies.Please confirm this is the intended approach for this repository. If generated code should not be checked in, consider:
- Adding these paths to
.gitignore- Documenting the code generation process in the build system
29-200: Descriptor wiring and field mappings look correct.The
Apiclass correctly:
- Declares descriptors for all v1 message types
- Embeds the serialized proto descriptor
- Wires dependencies (StructProto, EmptyProto, TimestampProto, AnnotationsProto)
- Sets up field accessor tables with correct field names
The new
PaginatorandPageInfotypes are properly integrated, supporting the pagination improvements.sdk/java/src/java/studio/o7/octopus/sdk/v1/GetRequestOrBuilder.java (1)
1-167: Generated interface correctly exposes pagination support.The
GetRequestOrBuilderinterface properly exposes the newpaginatorfield (lines 141-166), enabling clients to specify page-based pagination parameters. All optional fields correctly have presence checks.sdk/java/src/java/studio/o7/octopus/sdk/v1/EventCall.java (1)
1-781: Generated protobuf message class is correct.The
EventCallclass is a properly generated protobuf message with:
- Correct field accessors for
objectandcall_id- Standard serialization/deserialization logic
- Builder pattern implementation
- Proper integration with
ListenMessageas the callback oneof alternativesdk/java/src/java/studio/o7/octopus/sdk/v1/PageInfo.java (2)
46-159: PageInfo provides comprehensive pagination metadata.The
PageInfomessage exposes developer-friendly pagination fields:
currentPage/currentPageSize- current positiontotalPages/totalItems- result set boundsnextPage/prevPage(optional) - navigation helpersThe optional fields correctly use presence checks (
hasNextPage(),hasPrevPage()) so clients can distinguish "no next page" from "page 0". This addresses the PR objective of making pagination more development-friendly.
1-922: Generated code is consistent with Go SDK.The Java
PageInfoimplementation matches the Go struct (fromsdk/go/v1/api.pb.golines 327-337), ensuring cross-language consistency for SDK consumers.sdk/java/src/java/studio/o7/octopus/sdk/v1/OctopusGrpc.java (1)
12-137: gRPC service scaffold and method wiring look correctThe service name, method descriptors, and async/blocking/future stubs are wired consistently with the Api descriptors and the v1 message types; I don’t see any correctness issues in the gRPC surface here.
sdk/java/src/java/studio/o7/octopus/sdk/v1/Entry.java (1)
52-142: Entry message structure aligns with described metadata model
Entrycleanly wraps the underlyingObjectwith createdAt, revision, and id, and the generated accessors/serialization look standard and consistent with the proto schema.sdk/java/src/java/studio/o7/octopus/sdk/v1/ListenMessage.java (1)
51-153: Oneof handling forcallback/registerlooks correctThe
msgCase_/msg_handling and associated accessors/builders correctly model the oneof betweenEventCallandListenRegister, which should behave as expected for the listen stream.sdk/java/src/java/studio/o7/octopus/sdk/v1/ListenRegister.java (1)
53-119: ListenRegister schema and docs are consistent and minimalKey pattern wildcard documentation and the
priorityfield wiring match the proto/Go definitions; the generated Java surface looks fine as-is.sdk/java/src/java/studio/o7/octopus/sdk/v1/GetRequest.java (1)
53-305: GetRequest shape matches the intended filtered, paginated GET APIThe keyPattern, temporal range filters, optional includeExpired/dataFilter, and
Paginatorfield are all wired consistently with the proto/Go definition and support the new page-based pagination model; generated methods look correct.
| require ( | ||
| google.golang.org/genproto/googleapis/api v0.0.0-20251213004720-97cd9d5aeac2 | ||
| google.golang.org/grpc v1.77.0 | ||
| google.golang.org/grpc v1.78.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check GitHub for gRPC v1.78.0 release information
# Try to get release notes from GitHub
echo "=== Checking gRPC Go v1.78.0 release notes ==="
curl -s -H "Accept: application/vnd.github.v3+json" \
"https://api.github.com/repos/grpc/grpc-go/releases/tags/v1.78.0" | \
jq -r '.tag_name, .name, .published_at, .body' 2>/dev/null || echo "Could not fetch release info"Repository: o7studios/octopus-sdk
Length of output: 2036
🌐 Web query:
gRPC Go v1.78.0 release notes changelog security
💡 Result:
Release v1.78.0 (published 23 Dec 2025) — release notes show behavior changes, new features, bug fixes and performance improvements but do not list any security fixes or CVE patches in that release. [1]
The notable gRPC‑Go HTTP/2 rapid‑reset DoS (CVE‑2023‑44487 / GHSA‑m425‑mq94‑257g) was fixed in earlier patch releases (1.56.3, 1.57.1, 1.58.3 and later) rather than in v1.78.0. [2][3]
Sources:
- grpc/grpc-go — Release 1.78.0 (GitHub). [1]
- gRPC‑Go HTTP/2 Rapid Reset vulnerability (GHSA / advisory). [2]
- Snyk CVE summary for CVE‑2023‑44487. [3]
🌐 Web query:
google.golang.org/grpc v1.78.0 breaking changes compatibility
💡 Result:
Summary of breaking/compatibility changes in google.golang.org/grpc v1.78.0
-
Behavior change (potential breaking): the gRPC client now rejects target URLs whose hostname contains unbracketed colons when running with Go 1.26+. This can break code that previously used such target formats. [1]
-
No other deliberate API removals called out in the v1.78.0 release notes; the release also adds stats/otel metrics features. For prior releases, minimum supported Go version was raised in earlier minor releases (e.g., v1.76.0 / v1.72.0), so verify the minimum Go toolchain required by your dependency graph before upgrading. [1][2]
Recommendation: test your codepaths that construct client target URLs (and any custom resolvers) under v1.78.0; if you rely on hostnames with unbracketed colons, update them to use bracketed IPv6 literals or otherwise canonicalize the target. See the v1.78.0 release notes for details. [1]
Sources:
[1] grpc-go v1.78.0 release notes (summary in dependency bump PRs).
[2] grpc-go v1.78.x branch / repo metadata.
🌐 Web query:
gRPC v1.78.0 CVE security advisories vulnerabilities
💡 Result:
gRPC v1.78.0 — relevant public CVEs and notes
- CVE-2024-7246 — HPACK table poisoning (affects gRPC C++, Python, Ruby). Fixed in earlier upstream releases (see fixed versions list). Confirmed in NVD/vendor advisories. [1][2]
- CVE-2024-11407 — data-corruption (gRPC‑C++ transmit zero-copy). Described/fixed upstream (commit e9046b2…). [3][4]
Notes: the v1.78.0 release notes do not list new CVE fixes beyond routine bug fixes; you should verify your build/package vendor for backports or distro fixes and update to a release that includes the upstream fixes (or apply vendor patches). [5]
Sources:
- NVD — CVE-2024-7246. [turn1search3]
- CVE details / advisories for CVE-2024-7246. [turn0search5]
- NVD — CVE-2024-11407. [turn1search0]
- Ubuntu / SUSE advisories for CVE-2024-11407. [turn1search1][turn1search6]
- gRPC v1.78.0 release announcement. [turn2search2]
Test hostname validation behavior in Go 1.26+ environments.
The gRPC upgrade to v1.78.0 (released 2025-12-23) includes a behavior change: the client now rejects target URLs with unbracketed colons in the hostname when running with Go 1.26+. If your code constructs client targets using IPv6 addresses or custom resolvers, verify they use properly bracketed addresses (e.g., [::1]:8080 not ::1:8080) to avoid connection failures.
🤖 Prompt for AI Agents
In go.mod around line 7, the updated grpc dependency v1.78.0 introduces hostname
validation on Go 1.26+ that rejects unbracketed colons in target hostnames;
audit code that builds gRPC client target strings (including any custom
resolvers) and ensure IPv6 addresses are wrapped in brackets when a port is
present (e.g., [::1]:8080), update any helper that concatenates host:port to
detect IPv6 and add brackets, add unit/integration tests that construct targets
with IPv6 addresses and run under Go 1.26+, and update any docs or comments to
require bracketed IPv6 literals for gRPC targets.
| // 1-based page index (page=1 is first page) | ||
| int32 page = 1; | ||
|
|
||
| // Number of items to return. | ||
| int32 page_size = 1; | ||
| int32 page_size = 2; | ||
| } |
There was a problem hiding this comment.
Breaking change: Paginator message restructured.
The Paginator message has been restructured:
- New
pagefield added at tag 1 (1-based page index) page_sizemoved from tag 1 to tag 2
This is a wire-format breaking change. Existing clients sending pagination with page_size at tag 1 will have that value misinterpreted as page by the new schema.
If this breaking change is intentional, document the migration path clearly.
| public static final int PAGE_INFO_FIELD_NUMBER = 2; | ||
| private studio.o7.octopus.sdk.v1.PageInfo pageInfo_; | ||
| /** | ||
| * <pre> | ||
| * Page information's about the cursor. | ||
| * </pre> | ||
| * | ||
| * <code>.octopus_sdk.v1.PageInfo page_info = 2 [json_name = "pageInfo"];</code> | ||
| * @return Whether the pageInfo field is set. | ||
| */ | ||
| @java.lang.Override | ||
| public boolean hasPageInfo() { | ||
| return ((bitField0_ & 0x00000001) != 0); | ||
| } | ||
| /** | ||
| * <pre> | ||
| * Page information's about the cursor. | ||
| * </pre> | ||
| * | ||
| * <code>.octopus_sdk.v1.PageInfo page_info = 2 [json_name = "pageInfo"];</code> | ||
| * @return The pageInfo. | ||
| */ | ||
| @java.lang.Override | ||
| public studio.o7.octopus.sdk.v1.PageInfo getPageInfo() { | ||
| return pageInfo_ == null ? studio.o7.octopus.sdk.v1.PageInfo.getDefaultInstance() : pageInfo_; | ||
| } | ||
| /** | ||
| * <pre> | ||
| * Page information's about the cursor. | ||
| * </pre> | ||
| * | ||
| * <code>.octopus_sdk.v1.PageInfo page_info = 2 [json_name = "pageInfo"];</code> | ||
| */ | ||
| @java.lang.Override | ||
| public studio.o7.octopus.sdk.v1.PageInfoOrBuilder getPageInfoOrBuilder() { | ||
| return pageInfo_ == null ? studio.o7.octopus.sdk.v1.PageInfo.getDefaultInstance() : pageInfo_; | ||
| } |
There was a problem hiding this comment.
PageInfo doc still mentions “cursor” instead of page-based pagination
The comment for page_info (“Page information's about the cursor.”) no longer matches the page-based model exposed by PageInfo (currentPage, totalPages, nextPage, etc.). To avoid confusing SDK consumers, update the doc comment in v1/api.proto to describe page-based pagination (e.g., “Pagination metadata for the current page”) and regenerate the Java code.
🤖 Prompt for AI Agents
In sdk/java/src/java/studio/o7/octopus/sdk/v1/GetResponse.java around lines 114
to 150, the generated doc comment for page_info still says "Page information's
about the cursor." which is inaccurate for page-based pagination; update the
comment in the source proto (v1/api.proto) for the page_info field to something
like "Pagination metadata for the current page" (or similar describing
currentPage/totalPages/nextPage), then re-run the protobuf/Java code generator
to regenerate GetResponse.java so the corrected doc appears in this file.
| "description": "Cursor for efficient pagination. First page is empty." | ||
| } | ||
| }, | ||
| "description": "*\nPaginator for database entry cursor-pagination." |
There was a problem hiding this comment.
Description inconsistency: still references "cursor-pagination".
The Paginator description says "cursor-pagination" but this PR changes the model to page-based pagination. Update the proto source comment to reflect the new pagination approach (e.g., "Paginator for database entry page-based pagination") and regenerate.
🤖 Prompt for AI Agents
In sdk/openapi/octopus.swagger.json around line 346, the Paginator description
still references "cursor-pagination" but the model was changed to page-based
pagination; update the proto source comment to say something like "Paginator for
database entry page-based pagination" (or equivalent accurate wording), then
regenerate the OpenAPI/Swagger JSON so the updated comment flows into
sdk/openapi/octopus.swagger.json.
…-improved-paggination # Conflicts: # sdk/java/src/java/build.gradle.kts
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Makefilesdk/java/src/java/.gitignoresdk/java/src/java/build.gradle.kts
✅ Files skipped from review due to trivial changes (1)
- sdk/java/src/java/.gitignore
🔇 Additional comments (2)
sdk/java/src/java/build.gradle.kts (1)
29-30: Verify that this metadata change is intentional.The PR objectives focus on improving pagination behavior (resolves #45), but these changes update developer attribution metadata. Please confirm whether this change was intentionally included in this PR or should be moved to a separate commit/PR.
Makefile (1)
2-2: Fix Java SDK path in cleanup command.The Makefile cleans
./sdk/java/studio, but buf.gen.yaml generates Java output tosdk/java/src/java. These don't match, which means generated Java files won't be cleaned before regeneration and stale files could accumulate. Change line 2 to clean the correct path:- rm -rf ./sdk/java/studio && rm -rf ./sdk/go && rm -rf ./sdk/ts/src/gen && rm -rf ./sdk/openapi && buf generate --template buf.gen.yaml + rm -rf ./sdk/java/src/java && rm -rf ./sdk/go && rm -rf ./sdk/ts/src/gen && rm -rf ./sdk/openapi && buf generate --template buf.gen.yamlThe other paths (go, ts/src/gen, openapi) correctly match buf.gen.yaml outputs.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
proto/v1/api.proto (2)
138-143: Breaking change: Paginator message restructured.The
Paginatormessage has been restructured withpage_sizemoved from tag 1 to tag 2, and a newpagefield added at tag 1. This is a wire-format breaking change where existing clients sendingpage_sizeat tag 1 will have that value misinterpreted aspageby the new schema.
163-164: Update comment terminology and verify wire compatibility.The comment says "about the cursor" but this PR migrates from cursor-based to page-based pagination. Update the comment to reflect the new pagination model.
Additionally, replacing the previous
next_cursorfield (string) withpage_info(PageInfo message) at tag 2 is a wire-format breaking change. Existing clients expecting a string at tag 2 will fail to parse the response.🔎 Suggested fix:
- // Page information about the cursor. + // Page information about the response. PageInfo page_info = 2;Wire-unsafe changes will break if you parse data serialized using the old schema with a parser using the new schema. Avoid changing the wire type of any field; if you need to change a field type, deprecate the existing field and add a new field.
sdk/openapi/octopus.swagger.json (3)
251-254: Update description terminology in proto source.The description says "about the cursor" but this PR migrates to page-based pagination. Since this is generated from the proto file, fix the comment in
proto/v1/api.protoline 163 to say "Page information about the response" and regenerate this file.
315-318: Type inconsistency for totalItems.The
totalItemsfield has type "string" with format "int64". For OpenAPI/Swagger, integer values should use type "integer" with format "int64", not "string". This inconsistency may confuse API consumers and code generators.Check the protobuf-to-OpenAPI generation tool configuration to ensure
int64proto fields are correctly mapped to integer types in the OpenAPI spec.
346-346: Description inconsistency: still references "cursor-pagination".The Paginator description says "cursor-pagination" but this PR changes to page-based pagination. Since this is generated from the proto file, update the comment in
proto/v1/api.protoline 135 to reflect the new pagination approach (e.g., "Paginator for database entry page-based pagination") and regenerate this file.
🧹 Nitpick comments (1)
proto/v1/api.proto (1)
145-154: LGTM with minor suggestion for clarity.The new
PageInfomessage provides comprehensive pagination metadata. The field comments correctly use "page number" terminology and appropriately indicate thatnext_pageandprev_pageare optional.Consider adding a brief comment explaining that
next_pageandprev_pagewill be absent (null/not set) when there is no next or previous page respectively, for improved clarity.🔎 Optional clarification enhancement
- optional int32 next_page = 5; // next page number. null if not present - optional int32 prev_page = 6; // previous page number. null if not present + optional int32 next_page = 5; // next page number. Absent when on last page. + optional int32 prev_page = 6; // previous page number. Absent when on first page.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumsdk/go/v1/api.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
go.modproto/v1/api.protosdk/openapi/octopus.swagger.json
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🔇 Additional comments (4)
sdk/openapi/octopus.swagger.json (2)
4-4: LGTM: Title updated to reflect new proto path.The API title has been updated to align with the new proto file structure.
76-90: LGTM: Query parameters updated for page-based pagination.The paginator query parameters have been correctly updated to reflect the new page-based pagination model with appropriate types and descriptions.
proto/v1/api.proto (2)
3-7: Package paths restructured, but no breaking references in this repository.The protobuf package names and language-specific options have been changed:
- Go package alias:
apiv1→octopusv1- Java package:
studio.o7.octopus.sdk.gen.api.v1→studio.o7.octopus.sdk.v1- Proto package:
api.v1→octopus_sdk.v1No existing references to the old package paths were found in this repository. If external clients depend on the generated stubs from these packages, they will need to update their imports accordingly.
Likely an incorrect or invalid review comment.
85-88: The Object message fieldsexpired_atanddeleted_athave not been renumbered. They were created with tags 3 and 4 respectively in commit 7973570 and have not been modified since. There is no breaking change in this code.Likely an incorrect or invalid review comment.
Resolves: #45
Summary by CodeRabbit
Documentation
Refactor
Chores
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.