Skip to content

chore: [SDK-5531] add sticky log view to demo app - #905

Open
fadi-george wants to merge 1 commit into
mainfrom
fadi/sdk-5531
Open

fadi-george wants to merge 1 commit into
mainfrom
fadi/sdk-5531

Conversation

@fadi-george

Copy link
Copy Markdown
Collaborator

Description

One Line Summary

Add the sticky Log View to the Unity demo app and route SDK listener events through it.

Details

Motivation

The shared Appium suite (sdk-shared) asserts on demo log entries (log_view, log_entry_{index}) to verify JS/C# events fire exactly once, e.g. notification click and foregroundWillDisplay in specs 02 and 13. The Unity demo had no Log View, so those assertions were skipped. This brings Unity in line with sdk-shared/demo/build.md (Prompt 1.5, 2.0, 6.2) and styles.md (Log View), matching the Capacitor and .NET demos.

Scope

Demo app only, no SDK changes.

  • LogService: in-memory, max 100 entries, never persisted. Show/Error add to the view, Info is console only.
  • LogView: UI Toolkit panel pinned under the app bar with LOGS (n) header, clear and collapse buttons, empty state, auto-scroll to newest. Ids: log_view, log_entry_{index}, log_clear_button, log_toggle_button.
  • SDK listeners now log the exact build.md messages. Fixes IAM click (was logging action id) and notification click (was missing notification id).
  • Each entry is a time Label plus a plain-text message Label, since the a11y bridge reports Label.text raw and rich-text tags would leak into Appium reads.

Known gaps: the a11y bridge has no accessibility label support, so Clear logs / Collapse logs labels are not exposed (tests use ids). UI Toolkit has no line-height, so the 1.4 line height is not applied.

Testing

Manual testing

Ran run-local.sh --sdk=unity --spec="tests/specs/{02_,13_}*" on both platforms, all passing:

  • Android emulator (Android 17): 02 3/3, 13 2/2
  • iOS simulator (iPhone 17, iOS 26.5): 02 3/3, 13 2/2

Confirmed the log assertions ran instead of skipping: UiAutomator2 and XCUITest both find log_view and log_entry_* with the expected Notification click: {id} {title} text after the runs.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@fadi-george
fadi-george requested a review from a team as a code owner October 23, 2026 22:34

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Multi-model review

Claude Opus 5.5, GPT 5.6 Sol, Grok 4.7 on the demo sticky Log View (log_view / log_entry_* for Appium).

Act on: none

Consider (Opus+Grok): AccessibilityBridge.IsVisible crops to the 100px log ScrollView and treats DisplayStyle.None as hidden. After a few startup lines — or after collapse — older log_entry_{i} nodes go inactive/GONE. The UI Toolkit tree still has them, but getLogMessages / expectOneNewLog may not. Specs 02 and 13 passing does not cover a filled viewport. Same clip behavior as the other demo ports.

Consider (Opus+Grok): LogService.Show runs Changed → Render synchronously before e.Notification.Display() and NotifyStateChanged(). A throw in the rebuild or a11y resync would skip those side effects. The old Debug.Log could not.

Noted: Info/Error are unused; user-action traces stay on Debug.Log (matches the stated Show-vs-Info split). Trailing space when notification Title is null. \u2192 may be missing from DroidSansMono (visual only). DateTime.Now time separator is culture-sensitive (Appium reads the message label, not the time).

Dismissed: logout always calling FetchUserDataFromApi (pre-existing; that method already returns on an empty id and finally clears loading). Off-thread list/UI writes (these listeners already hop via UnityMainThreadDispatch). Attach/detach leak (paired). Clear vs Render race (same main thread).

GPT had no findings.

Open in Web View Automation 

Sent by Cursor Automation: PR Reviews

}

.log-body {
height: 100px;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider (Opus+Grok): this 100px ScrollView plus AccessibilityBridge.IsVisible (clips to contentViewport; DisplayStyle.None is hidden) drops older log_entry_{i} from the Appium tree once startup lines wrap or the panel is collapsed. Nodes stay in UI Toolkit; UiAutomator/XCUITest may not see them. Specs 02/13 passing does not cover a filled viewport.

Comment on lines +192 to 195
LogService.Show(
$"Notification foregroundWillDisplay: {e.Notification.NotificationId ?? "null"} {e.Notification.Title ?? ""}"
);
e.Notification.Display();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider (Opus+Grok): Show invokes LogView.Render (and an a11y resync) before Display(). If that rebuild throws, the notification is never shown. Same pattern in OnPushSubscriptionChanged before NotifyStateChanged(). Worth calling the side effect first, or isolating the log write.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant