fix:soften longToNumber overflow to prevent stream teardown - #50
fix:soften longToNumber overflow to prevent stream teardown#50Ishaan Kesarwani (IshaanXCoder) wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe code-generation workflow now patches generated ChangesInt64 overflow fix
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
hey LingJueYa～ (@LingJueYa) checkout this pr, let me know if we need any fixes/changes here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/soften-int64.mjs (1)
49-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer template literals over string concatenation.
As per coding guidelines, use template literals instead of string concatenation to format the error message.
♻️ Proposed refactor
-if (patched === 0 && alreadySoft === 0) { - throw new Error( - "soften-int64: found no longToNumber helper under " + - ROOT + - " — ts-proto's emitted shape may have changed; update THROWING_BODY." - ); -} +if (patched === 0 && alreadySoft === 0) { + throw new Error( + `soften-int64: found no longToNumber helper under ${ROOT} — ts-proto's emitted shape may have changed; update THROWING_BODY.` + ); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/soften-int64.mjs` around lines 49 - 55, The error message in the soften-int64 failure check should use a template literal instead of concatenating strings. Update the Error construction within the patched/alreadySoft guard, preserving the existing message content and ROOT interpolation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/soften-int64.mjs`:
- Around line 49-55: The error message in the soften-int64 failure check should
use a template literal instead of concatenating strings. Update the Error
construction within the patched/alreadySoft guard, preserving the existing
message content and ROOT interpolation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a45b4f3-9bc2-4c8d-bea0-2ffcb92b390f
⛔ Files ignored due to path filters (9)
packages/core/src/generated/google/protobuf/descriptor.tsis excluded by!**/generated/**packages/core/src/generated/google/protobuf/timestamp.tsis excluded by!**/generated/**packages/core/src/generated/photon/imessage/v1/attachment_types.tsis excluded by!**/generated/**packages/core/src/generated/photon/imessage/v1/chat_service.tsis excluded by!**/generated/**packages/core/src/generated/photon/imessage/v1/event_service.tsis excluded by!**/generated/**packages/core/src/generated/photon/imessage/v1/group_service.tsis excluded by!**/generated/**packages/core/src/generated/photon/imessage/v1/location_types.tsis excluded by!**/generated/**packages/core/src/generated/photon/imessage/v1/message_service.tsis excluded by!**/generated/**packages/core/src/generated/photon/imessage/v1/poll_service.tsis excluded by!**/generated/**
📒 Files selected for processing (3)
package.jsonpackages/core/tests/long-to-number.test.tsscripts/soften-int64.mjs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions
Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, nevervar
Alwaysawaitpromises in async functions - don't forget to use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Keep functions focused and under reasonable cognitive complexity limits
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Group related code together and separate concerns
Don't useeval()or assign directly todocument.cookie
Validate and sanitize user input
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Avoid barrel files (index files that re-export everything)
Write assertions insideit()ortest()blocks`**/*.{ts...
Files:
packages/core/tests/long-to-number.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx,js,jsx}: Avoid done callbacks in async tests - use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat - avoid excessivedescribenesting
Files:
packages/core/tests/long-to-number.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use TypeScript for all new projects and modules
Files:
packages/core/tests/long-to-number.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Write comprehensive unit tests for all functions and components
Files:
packages/core/tests/long-to-number.test.ts
🔇 Additional comments (2)
package.json (1)
23-23: LGTM!packages/core/tests/long-to-number.test.ts (1)
1-45: LGTM!
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #23 by preventing generated proto decoders from throwing when an int64/uint64 exceeds Number.MAX_SAFE_INTEGER, avoiding gRPC stream teardown during event consumption.
Changes:
- Adds a post-codegen patch script to rewrite ts-proto’s
longToNumber()helper into a non-throwing (lossy) conversion. - Wires the patch script into
npm run generateto keepverify:codegenreproducible. - Adds a unit regression test ensuring oversized
int64/uint64values decode without throwing.
Reviewed changes
Copilot reviewed 3 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/soften-int64.mjs | New post-codegen script that replaces ts-proto’s throwing longToNumber() helper with a non-throwing version. |
| package.json | Runs the new soften script after buf generate to keep codegen deterministic. |
| packages/core/tests/long-to-number.test.ts | Regression tests for decoding oversized uint64 and Timestamp.seconds values without throwing. |
| packages/core/src/generated/photon/imessage/v1/poll_service.ts | Softens longToNumber() in generated poll service decoder paths. |
| packages/core/src/generated/photon/imessage/v1/message_service.ts | Softens longToNumber() in generated message service decoder paths. |
| packages/core/src/generated/photon/imessage/v1/location_types.ts | Softens longToNumber() in generated location types decoder paths. |
| packages/core/src/generated/photon/imessage/v1/group_service.ts | Softens longToNumber() in generated group service decoder paths. |
| packages/core/src/generated/photon/imessage/v1/event_service.ts | Softens longToNumber() in generated event service decoder paths. |
| packages/core/src/generated/photon/imessage/v1/chat_service.ts | Softens longToNumber() in generated chat service decoder paths. |
| packages/core/src/generated/photon/imessage/v1/attachment_types.ts | Softens longToNumber() in generated attachment types decoder paths. |
| packages/core/src/generated/google/protobuf/timestamp.ts | Softens longToNumber() used for Timestamp.seconds. |
| packages/core/src/generated/google/protobuf/descriptor.ts | Softens longToNumber() in generated descriptor types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
scripts/soften-int64.mjs:46
- The fallback
else if (contents.includes("function longToNumber"))treats anylongToNumberhelper as “already softened”. If ts-proto changes the emitted helper body (but it’s still throwing), this script will exit successfully without patching, and CI will keep publishing the throwing decoder. Consider only counting as already-soft when the softened body is present; otherwise fail fast soverify:codegencatches the drift.
} else if (contents.includes("function longToNumber")) {
alreadySoft++;
}
Summary
this pr fixes #23
scripts/soften-int64.mjs: post-codegen script, replaces the throwing helper body (identical across all 9 generated files) with a lossy-but-non-throwing version.generatesoverify:codegenstays reproducible in CI.number.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
Bug Fixes
Tests
Chores