Skip to content

[realppl 9] realppl public api and integration tests#14931

Closed
wu-hui wants to merge 1 commit intowuandy/RealPpl_8from
wuandy/RealPpl_9
Closed

[realppl 9] realppl public api and integration tests#14931
wu-hui wants to merge 1 commit intowuandy/RealPpl_8from
wuandy/RealPpl_9

Conversation

@wu-hui
Copy link
Contributor

@wu-hui wu-hui commented Jun 4, 2025

No description provided.

@gemini-code-assist
Copy link
Contributor

Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and 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 to provide feedback.

@google-oss-bot
Copy link
Collaborator

1 Warning
⚠️ Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger


@available(iOS 13, tvOS 13, macOS 10.15, macCatalyst 13, watchOS 7, *)
public struct PipelineSource: @unchecked Sendable {
public struct PipelineSource<P>: @unchecked Sendable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Andy, I remember in the discussion we decided to avoid using generic and go with RealtimePipelineSource ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some source, for example database is not available for the realtime ppl (although we can relay on runtime checking)

@wu-hui wu-hui force-pushed the wuandy/RealPpl_9 branch 2 times, most recently from f7273d4 to 676dc22 Compare June 14, 2025 08:28
@wu-hui wu-hui force-pushed the wuandy/RealPpl_8 branch from 0135d82 to 8139cf4 Compare June 30, 2025 18:37
@wu-hui wu-hui force-pushed the wuandy/RealPpl_9 branch from 676dc22 to 01df95f Compare June 30, 2025 18:37
@wu-hui wu-hui force-pushed the wuandy/RealPpl_8 branch from 8139cf4 to f164452 Compare July 3, 2025 17:38
@wu-hui wu-hui force-pushed the wuandy/RealPpl_9 branch 2 times, most recently from c33f64a to 020bfbc Compare July 7, 2025 17:19
@wu-hui wu-hui force-pushed the wuandy/RealPpl_8 branch from f164452 to 38b72bd Compare July 7, 2025 17:19
@wu-hui wu-hui force-pushed the wuandy/RealPpl_9 branch 2 times, most recently from 9a4b558 to fb16f96 Compare September 12, 2025 17:30
@wu-hui wu-hui force-pushed the wuandy/RealPpl_8 branch 2 times, most recently from 5a19c8f to 8b8d4a2 Compare September 12, 2025 18:30
@wu-hui wu-hui force-pushed the wuandy/RealPpl_9 branch 2 times, most recently from 3c2fbf7 to b9d0d1a Compare September 15, 2025 15:42
@wu-hui wu-hui force-pushed the wuandy/RealPpl_8 branch 2 times, most recently from bf5a534 to e059610 Compare September 17, 2025 15:06
@wu-hui wu-hui requested a review from ehsannas September 17, 2025 15:08
@wu-hui wu-hui force-pushed the wuandy/RealPpl_9 branch 2 times, most recently from c9c5990 to a99fe2e Compare September 19, 2025 16:45
@wu-hui wu-hui force-pushed the wuandy/RealPpl_8 branch 2 times, most recently from c6f2775 to 0e39951 Compare September 29, 2025 15:38
if (databaseId) {
defaultDatabaseId = databaseId;
} else {
defaultDatabaseId = @"enterprise";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the feature branch we use (void)switchToEnterpriseMode to turn on enterprise mode for testing. I think that way is better than using enterprise database id as default.

@cherylEnkidu
Copy link
Contributor

/gemini review

@cherylEnkidu cherylEnkidu assigned wu-hui and unassigned ehsannas Nov 20, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new realtime pipeline feature, including public APIs for Objective-C and Swift, and integration tests. The changes are extensive and well-structured. I've found a few issues to address. The Xcode project file has several duplicate entries which could cause build problems. There's also a critical bug in the new Swift API that could lead to a crash. Finally, a public API was removed, which might be a breaking change.

Comment on lines +95 to +104
return bridge.addSnapshotListener(options: options.bridge) { snapshotBridge, error in
listener(
RealtimePipelineSnapshot(
// TODO(pipeline): this needs to be fixed
snapshotBridge!,
pipeline: self
),
error
)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The force unwrap of snapshotBridge! will cause a crash if an error occurs and the snapshot is nil. The listener callback should handle the nil snapshot case gracefully when an error is present.

The TODO comment also indicates this might be known. This should be fixed before merging.

    return bridge.addSnapshotListener(options: options.bridge) { snapshotBridge, error in
      if let error = error {
        listener(nil, error)
        return
      }

      if let snapshotBridge = snapshotBridge {
        let snapshot = RealtimePipelineSnapshot(snapshotBridge, pipeline: self)
        listener(snapshot, nil)
      }
    }

101393F60336924F64966C74 /* globals_cache_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 4564AD9C55EC39C080EB9476 /* globals_cache_test.cc */; };
1029F0461945A444FCB523B3 /* leveldb_local_store_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 5FF903AEFA7A3284660FA4C5 /* leveldb_local_store_test.cc */; };
10B69419AC04F157D855FED7 /* leveldb_document_overlay_cache_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AE89CFF09C6804573841397F /* leveldb_document_overlay_cache_test.cc */; };
11105C1A9E2065B6A3816983 /* pipeline_util_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 9F12A488C443DBCCEC54DB61 /* pipeline_util_test.cc */; };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The file pipeline_util_test.cc is added to the project multiple times under different build file sections (e.g., here and on lines 508, 897, etc.). This will likely cause build errors and should be corrected to a single entry.

Comment on lines +154 to +156
1296CECE2DEE97F5007F8552 /* RealtimePipelineTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1296CECD2DEE97EF007F8552 /* RealtimePipelineTests.swift */; };
1296CECF2DEE97F5007F8552 /* RealtimePipelineTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1296CECD2DEE97EF007F8552 /* RealtimePipelineTests.swift */; };
1296CED02DEE97F5007F8552 /* RealtimePipelineTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1296CECD2DEE97EF007F8552 /* RealtimePipelineTests.swift */; };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There are duplicate entries for RealtimePipelineTests.swift in the build files. This can cause build issues and should be cleaned up. Only one entry is needed.

Comment on lines +27 to -30
return factory([CollectionSource(collection: normalizedPath)], db)
}

public func collection(_ ref: CollectionReference) -> Pipeline {
let collectionStage = CollectionSource(collection: ref, db: db)
return Pipeline(stages: [collectionStage], db: db)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The collection(_ ref: CollectionReference) function was removed. Was this intentional? While users can still use collection(ref.path), this is a breaking change and less convenient. If this was not intentional, it should be restored.

@ncooke3
Copy link
Member

ncooke3 commented Jan 9, 2026

@wu-hui , I'm doing some review of open PRs. Can this be closed now that pipelines is in main?

@wu-hui wu-hui closed this Jan 9, 2026
@paulb777 paulb777 deleted the wuandy/RealPpl_9 branch February 18, 2026 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants