feat(swift-example-app): document sum/average aggregation view (DOC-13/14)#3942
Conversation
…3/14) Add a read-only "Sum / Average Documents" view that drives the document SUM and AVERAGE aggregation FFI shipped in #3935, moving QA tests DOC-13 (sum) and DOC-14 (average) from SDK-only to builder-only (drivable in the simulator). Mirrors the COUNT view added in #3926 for DOC-10/11/12. - Add thin-bridge wrappers SDK.documentSum / SDK.documentAverage over the dash_sdk_document_sum / dash_sdk_document_average FFI, plus the DocumentSumResult / DocumentAverageEntry / DocumentAverageResult result types. The FFI returns the raw (count, sum) pair; the sum/count division for display stays in the view (the wrappers only marshal). - Add SumAverageDocumentsView: op selector (Sum/Average), a required numeric sum-property field, optional where/group_by JSON, and result rendering (total + per-group rows, computed average for the avg op). - Wire it next to the Count link under Settings -> Platform State Transitions -> Document. - Flip DOC-13/14 to builder-only in TEST_PLAN.md and drop them from the SDK-only gaps list. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
llbartekll and
shumkov
as code owners
|
Warning Review limit reached
More reviews will be available in 37 minutes and 6 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ 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 |
|
✅ Review complete (commit d4275e9) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Additive Swift wrapper + SwiftExampleApp view exposing the already-merged document SUM/AVERAGE FFI (#3935) for QA DOC-13/14. The JSON-payload shapes, contract-handle fetch/destroy lifecycle, hex-key/empty-key convention, and presentation-layer division of (sum/count) all match the existing documentCount precedent. No Rust changes, no consensus-critical paths, no FFI surface added. One minor UX nit on the result rendering of grouped queries.
💬 1 nitpick(s)
| private func sumResultSections(_ result: DocumentSumResult) -> some View { | ||
| Section("Total") { | ||
| HStack { | ||
| Text("Sum") | ||
| Spacer() | ||
| Text(result.total.map(String.init) ?? "—") | ||
| .fontWeight(.bold) | ||
| .foregroundColor(.primary) | ||
| .accessibilityIdentifier("sumAverageDocuments.total") | ||
| } | ||
| } | ||
|
|
||
| if result.isGrouped { | ||
| Section("Per-group sums") { | ||
| ForEach(groupedSumRows(result), id: \.key) { row in | ||
| HStack { | ||
| Text(row.key) | ||
| .font(.system(.footnote, design: .monospaced)) | ||
| .lineLimit(1) | ||
| .truncationMode(.middle) | ||
| Spacer() | ||
| Text(String(row.value)) | ||
| .fontWeight(.semibold) | ||
| } | ||
| .accessibilityIdentifier("sumAverageDocuments.groupRow.\(row.key)") | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @ViewBuilder | ||
| private func averageResultSections(_ result: DocumentAverageResult) -> some View { | ||
| Section("Total") { | ||
| HStack { | ||
| Text("Average") | ||
| Spacer() | ||
| Text(formatAverage(result.total)) | ||
| .fontWeight(.bold) | ||
| .foregroundColor(.primary) | ||
| .accessibilityIdentifier("sumAverageDocuments.average") | ||
| } | ||
| if let entry = result.total { | ||
| HStack { | ||
| Text("Count") | ||
| Spacer() | ||
| Text(String(entry.count)) | ||
| .foregroundColor(.secondary) | ||
| } | ||
| HStack { | ||
| Text("Sum") | ||
| Spacer() | ||
| Text(String(entry.sum)) | ||
| .foregroundColor(.secondary) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: "Total" section renders "—" above per-group rows for grouped queries
DocumentSumResult.total and DocumentAverageResult.total resolve to nil when the request was grouped (no empty-string entry in the FFI payload — see PlatformQueryExtensions.swift:47-49 and :97-99). However, sumResultSections and averageResultSections always emit the Section("Total") block, so a grouped run renders a misleading "Sum: —" / "Average: —" above the real per-group rows. (For averages, the Count/Sum sub-rows are correctly gated by if let entry = result.total, so those stay hidden — just the "Average: —" line is the issue.) Gate the Total section on !result.isGrouped, or label it explicitly so QA isn't reading a placeholder dash as a query failure. Purely UX; does not affect the accessibility identifiers QA depends on.
source: ['claude']
|
Companion fix: #3944 — the example-app SDK was pinned to protocol version 11 on testnet (V0 wire), which rejects the V1-only Sum/Average projection this view drives. With #3944 the SDK seeds at the per-network floor (testnet PV12), and DOC-13/14 return proof-verified results in-app (sum 120 / average 30 over a summable fixture). #3944 should land for the testnet green path. |
2 participants
Issue being fixed or feature implemented
The document SUM/AVERAGE aggregation FFI (
dash_sdk_document_sum/dash_sdk_document_average) shipped in #3935, but SwiftExampleApp had no way to invoke it — so QA tests DOC-13 (sum) and DOC-14 (average) were stuck at 🔌 SDK-only (FFI exists, no UI to drive it), unlike the COUNT tests DOC-10/11/12 which got a view in #3926.This adds the missing app surface so those two tests become 🧪 builder-only (drivable in the simulator).
What was done?
PlatformQueryExtensions.swift): addedSDK.documentSum(...)andSDK.documentAverage(...)as thin bridges over the merged FFI, plus theDocumentSumResult/DocumentAverageEntry/DocumentAverageResultresult types. These mirror the existingdocumentCount/DocumentCountResultprecedent exactly (contract-handle fetch + defer destroy,withOptionalCStringmarshalling,limit == 0guard,SDKError.fromDashSDKError). The only new wire detail is the required non-emptysum_propertyargument. Per the Swift SDK thin-bridge rule, the wrappers only marshal — the FFI returns the raw(count, sum)pair and thesum / countdivision for display is done in the view.SumAverageDocumentsView.swift: a read-only query view (nothing signed/broadcast) mirroringCountDocumentsView. It has an op selector (Sum / Average), a required numeric sum-property field, optionalwhere/group_byJSON, and renders the total + per-group rows (and the computed average for the avg op) or the platform error.TransitionCategoryView.swift): aNavigationLinknext to the Count link under Settings → Platform State Transitions → Document.getDocumentscatalog row and Appendix B SDK-only gaps list updated accordingly.No Rust source was changed; the FFI was already merged in #3935.
Accessibility identifiers (for idb-driven QA)
transition.document.sumAverageDocuments(nav link); within the view:sumAverageDocuments.opPicker,.sumPropertyField,.whereField,.groupByField,.runButton,.total(sum),.average(computed),.groupRow.<hexKey>,.errorText,.contractPicker/.docTypePicker.How Has This Been Tested?
./build_ios.sh --target sim --profile dev) so the regeneratedDashSDKFFIheader carries the newdash_sdk_document_sum/dash_sdk_document_averagesymbols, then built SwiftExampleApp for the iPhone 16 simulator with warnings-as-errors → BUILD SUCCEEDED. Verified the new symbols are present in the generated header and thatsumAverageDocumentsstrings are present in the linked dylib (no stale-binary trap).summableindex on a numeric property (the existingcountableQA fixture is insufficient — counting only needsdocumentsCountable). Until such a fixture is published, the view correctly surfaces the platform's "requires a summable index" error. This is a data/fixture concern, not a code one.Breaking Changes
None. Additive SDK wrapper + new example-app view.
Checklist:
For repository code-owners and collaborators only