Stripe + partial work on browser events query

[dinmukhamedm]

Important

Add Stripe subscription management and refactor backend code for cleaner architecture and improved browser events handling.

• Stripe Integration:
  • Add Stripe subscription management in checkout/page.tsx, portal/page.tsx, and webhook/route.ts.
  • Implement getUserSubscriptionInfo and manageSubscriptionEvent in checkout/utils.ts.
  • Update features.ts to include SUBSCRIPTION feature flag.
• Backend Changes:
  • Remove subscriptions.rs and related routes from main.rs.
  • Refactor MessageQueue and SemanticSearch to use enum_dispatch for cleaner code.
  • Remove zstd dependency from Cargo.toml and Cargo.lock.
• Browser Events:
  • Update process_browser_events in browser_events/mod.rs to handle message deserialization errors.
  • Use serde_json::value::RawValue for RRWebEvent data in browser_sessions.rs.
• Miscellaneous:
  • Update CacheTrait to remove unnecessary Send + Sync bounds in cache/mod.rs.
  • Refactor CodeExecutor and MachineManager to use traits for implementation.

^{This description was created by for 96f2312. It will automatically update as commits are pushed.}

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 96f2312 in 2 minutes and 54 seconds

More details

• Looked at 2787 lines of code in 54 files
• Skipped 0 files when reviewing.
• Skipped posting 19 drafted comments based on config settings.

1. app-server/src/api/v1/browser_sessions.rs:62

• Draft comment:
  Good use of serde_json::to_vec for serializing the message. Consider handling errors instead of using unwrap so that a serialization failure doesn’t cause a panic.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
  The code is using unwrap() on JSON serialization which could theoretically panic. However, we can see that the message struct derives Serialize and contains only basic types (UUID and EventBatch which also only contains basic types). Serialization of these types cannot fail under normal circumstances. The change introduced this unwrap call, so it is about changed code.
  I might be overlooking some edge case where serialization could fail. The custom types involved could have custom serialization implementations I can't see.
  While custom serialization could theoretically fail, the types here are simple and standard. More importantly, if serialization fails it indicates a serious programming error that should be caught in testing.
  While the comment identifies a real unwrap(), this is a case where unwrap() is acceptable since serialization cannot fail unless there's a serious programming error.

2. app-server/src/api/v1/machine_manager.rs:22

• Draft comment:
  Using Arc improves clarity. Verify that both production and mock implementations correctly handle their error paths to avoid runtime panics.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to verify the handling of error paths, which falls under the category of asking for confirmation or verification. This violates the rule against asking the PR author to confirm or ensure behavior. Therefore, this comment should be removed.

3. app-server/src/api/v1/semantic_search.rs:60

• Draft comment:
  Parsing UUIDs via serde_json::from_str may panic if the input is unexpected. Consider adding explicit error handling and context.
• Reason this comment was not posted:
  Comment was on unchanged code.

4. app-server/src/traces.rs:60

• Draft comment:
  Keep-alive header handling is well implemented. Ensure that decoding errors for the ExportTraceServiceRequest are logged with sufficient detail.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. app-server/src/mq/rabbit.rs:70

• Draft comment:
  Channel creation for every publish call might be inefficient under load. Consider refactoring to cache and reuse channels.
• Reason this comment was not posted:
  Comment was on unchanged code.

6. app-server/src/api/v1/traces.rs:69

• Draft comment:
  Replace unwrap() in the publish call with proper error handling. Unwrapping the serialization result may panic if serialization fails.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

7. app-server/src/storage/s3.rs:15

• Draft comment:
  The get_url function assumes the key always has the prefix 'project/'. Consider handling cases where the key format is unexpected instead of unconditionally calling unwrap().
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

8. app-server/src/db/spans.rs:30

• Draft comment:
  Avoid using unwrap() for parsing UUIDs from OTel span data. Consider propagating the error so that invalid trace/span IDs are handled gracefully.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. app-server/src/semantic_search/semantic_search_impl.rs:51

• Draft comment:
  The model is hard-coded to 'CohereMultilingual'. Consider parameterizing the model selection to allow flexibility or configuration for different deployments.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

10. app-server/src/traces/spans.rs:690

• Draft comment:
  Avoid using unwrap() in payload storage logic. Replace them with proper error handling to prevent panics in production if serialization fails.
• Reason this comment was not posted:
  Comment was on unchanged code.

11. app-server/src/machine_manager/mod.rs:78

• Draft comment:
  The MockMachineManager implementation uses todo!() for all methods. While acceptable in a stub, consider at least returning a Result with a clear error until full implementation is provided.
• Reason this comment was not posted:
  Comment was on unchanged code.

12. frontend/lib/checkout/utils.ts:124

• Draft comment:
  When updating the usage cache, consider adding error handling or logging in case the cache set operation fails.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The code author has explicitly documented that cache failures are acceptable and explained the worst-case scenario. Adding error handling would add complexity without meaningful benefit since the failure mode is already understood and accepted. The comment seems to be suggesting a best practice without considering the specific context.
  Perhaps there could be value in logging cache failures for monitoring purposes, even if we don't handle them differently. The comment might be identifying a legitimate observability gap.
  While logging could be useful, the comment as written focuses on error handling which isn't needed here. If observability was the concern, the comment should have been more specific about that.
  Delete the comment. The code author has explicitly documented that cache failures are acceptable, making additional error handling unnecessary.

13. frontend/app/checkout/page.tsx:18

• Draft comment:
  Ensure that the lookupKey default is updated consistently with backend changes. It's set to 'pro_monthly_2025_02' which must match backend configurations.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  This is a "verify/ensure" type comment that asks the author to double-check something. It's speculative since it assumes there might be a mismatch without evidence. The rules explicitly say not to make comments asking authors to verify things. If there was an actual mismatch, it would be caught when testing the checkout flow.
  Maybe this is actually a critical configuration that needs to be in sync, and the comment is preventing a serious production issue?
  Even if synchronization is important, this would be caught during testing - the checkout flow would fail if the lookupKey doesn't exist. We should trust that the author tested their changes.
  Delete this comment as it violates our rules against asking authors to verify things and making speculative comments without evidence of actual issues.

14. frontend/lib/features/features.ts:36

• Draft comment:
  The feature flag for SUBSCRIPTION relies on STRIPE_SECRET_KEY. Verify that this condition appropriately reflects your production settings and consider documenting the rationale behind the check.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to verify their production settings and to document the rationale behind a check. This violates the rule against asking the author to confirm their intention or to ensure behavior is intended. It also suggests documenting rationale, which is not allowed.

15. app-server/src/api/v1/semantic_search.rs:82

• Draft comment:
  Typo: In line 82, the key 'datasource_id' is used when it looks like it should be 'dataset_id' to be consistent with the rest of the code (e.g., in request and DB calls). Please verify and correct this if it's unintended.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

16. app-server/src/cache/mod.rs:16

• Draft comment:
  It looks like the error attribute for SerDeError is using double curly braces ("{{0}}"). Typically, you would want to use single curly braces ("{0}") to interpolate the argument. Please verify if this is intentional or an unintentional typo.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

17. app-server/src/traces/consumer.rs:38

• Draft comment:
  Typographical error: 'queue conneciton' should be corrected to 'queue connection'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

18. app-server/src/traces/consumer.rs:113

• Draft comment:
  Typographical/consistency issue: The error message 'Failed to store input images.' appears to be a copy-paste mistake. Consider revising it to accurately reflect the operation being performed (e.g., 'Failed to store span payloads.').
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

19. app-server/src/traces/producer.rs:35

• Draft comment:
  Typographical error: The comment on line 35 says 'OpenLLMetry auto-instrumentation sends this event for every chunk'. Please change 'OpenLLMetry' to 'OpenTelemetry'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_JnuQfvuO58X5qUCN

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

Avoid using unwrap() when serializing messages. Consider propagating errors via ? so that issues in serialization can be caught and handled gracefully.

Suggested change

	&serde_json::to_vec(&message).unwrap(),
	&serde_json::to_vec(&message)?,

[dinmukhamedm]

Good catch, but this will very soon be re-written, and seems to be working fine now