Skip to content

wip vendor unused connection observer - #1589

Merged
lebaudantoine merged 1 commit into
mainfrom
connection-observer
Aug 13, 2026
Merged

wip vendor unused connection observer#1589
lebaudantoine merged 1 commit into
mainfrom
connection-observer

Conversation

@lebaudantoine

Copy link
Copy Markdown
Collaborator

No description provided.

@lebaudantoine

Copy link
Copy Markdown
Collaborator Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Remove unused connection observer candidate metadata plumbing

✨ Enhancement 🕐 10-20 Minutes

Grey Divider

AI Description

• Remove peer-connection candidate polling and related feature-flag logic from ConnectionObserver.
• Stop attaching pc_publisher/pc_subscriber metadata to feedback navigation and rating analytics.
• Simplify connectionObserverStore to only track the idle-disconnect modal state.
Diagram

graph TD
A["Conference"] --> B["Feedback route"] --> C["Rating"] --> D["Telemetry"]
E["ConnectionObserver"] --> F[("connectionObserverStore")]
E --> D
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Keep candidate polling but log directly on disconnect (no global store)
  • ➕ Avoids storing potentially sensitive candidate details in global state
  • ➕ Keeps implementation closer to the only consumer (disconnect telemetry)
  • ➖ Still relies on LiveKit internals (pcManager publisher/subscriber)
  • ➖ More complex than removing unused functionality
2. Replace polling with event-driven stats sampling
  • ➕ Lower overhead than periodic getStats polling
  • ➕ Potentially more accurate change detection tied to network events
  • ➖ May not be feasible depending on available WebRTC/LiveKit hooks
  • ➖ More engineering effort than warranted if data is currently unused
3. Remove connectionObserverStore entirely and use component-local state
  • ➕ Reduces global mutable state surface area
  • ➕ Easier to reason about lifecycle/cleanup
  • ➖ Requires refactoring any other consumers of isIdleDisconnectModalOpen
  • ➖ May complicate cross-component UI control if used elsewhere later

Recommendation: Given the candidate metadata was unused and required polling + LiveKit internals, removing it is the lowest-risk and most maintainable choice. If candidate-level diagnostics are needed later, prefer logging on disconnect without a global store, and keep it behind a clearly-scoped feature flag with explicit privacy review.

Files changed (5) +1 / -147

Refactor (5) +1 / -147
Conference.tsxDrop connection observer candidate metadata from disconnect navigation +0/-16

Drop connection observer candidate metadata from disconnect navigation

• Removes connectionObserverStore usage when disconnecting from a room. Feedback navigation state now only includes room_id, and candidate metadata is no longer reset on disconnect.

src/frontend/src/features/rooms/components/Conference.tsx

Rating.tsxRemove CandidateInfo-based metadata from rating payload type +0/-5

Remove CandidateInfo-based metadata from rating payload type

• Eliminates CandidateInfo imports and removes pc_* fields from the RatingMetadata type. Rating metadata is now limited to room_id (plus session_id computed locally).

src/frontend/src/features/rooms/components/Rating.tsx

ConnectionObserver.tsxRemove advanced candidate polling logic and related dependencies +1/-107

Remove advanced candidate polling logic and related dependencies

• Deletes the periodic WebRTC getStats candidate-pair polling, mobile/feature-flag gating, and associated imports. ConnectionObserver remains responsible for idle-disconnect modal timing and basic connection lifecycle telemetry.

src/frontend/src/features/rooms/livekit/components/ConnectionObserver.tsx

Feedback.tsxStop reading pc_* metadata from history state +0/-5

Stop reading pc_* metadata from history state

• Removes CandidateInfo typing and no longer extracts pc_publisher/pc_subscriber and change counts from window.history.state. Feedback now passes only room_id metadata to Rating.

src/frontend/src/features/rooms/routes/Feedback.tsx

connectionObserver.tsSimplify connectionObserverStore to only idle modal state +0/-14

Simplify connectionObserverStore to only idle modal state

• Removes CandidateInfo type and all publisher/subscriber candidate fields and counters from the store. The store now only tracks isIdleDisconnectModalOpen.

src/frontend/src/stores/connectionObserver.ts

@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 63b91796-19e9-4be3-b7ee-8ae224c2e39d

📥 Commits

Reviewing files that changed from the base of the PR and between 047a4c9 and 7399cf0.

📒 Files selected for processing (5)
  • src/frontend/src/features/rooms/components/Conference.tsx
  • src/frontend/src/features/rooms/components/Rating.tsx
  • src/frontend/src/features/rooms/livekit/components/ConnectionObserver.tsx
  • src/frontend/src/features/rooms/routes/Feedback.tsx
  • src/frontend/src/stores/connectionObserver.ts
💤 Files with no reviewable changes (4)
  • src/frontend/src/stores/connectionObserver.ts
  • src/frontend/src/features/rooms/components/Conference.tsx
  • src/frontend/src/features/rooms/components/Rating.tsx
  • src/frontend/src/features/rooms/routes/Feedback.tsx

Walkthrough

The frontend removes publisher and subscriber connection diagnostics from disconnect and feedback metadata. The connection observer store now tracks only the idle-disconnect modal state. ConnectionObserver no longer polls WebRTC statistics or tracks selected remote ICE candidate changes. Analytics connection events and idle-disconnect modal behavior remain.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive No pull request description was provided, so the purpose and scope of the connection observer changes are not documented. Add a brief description that explains the removal of candidate polling, connection metadata, and related store state.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title identifies the connection observer change but uses vague wording and does not clearly describe the removed polling and metadata.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@greptile-apps

greptile-apps Bot commented Aug 12, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
src/frontend/src/features/rooms/livekit/components/ConnectionObserver.tsx Removes feature-gated ICE candidate polling while retaining analytics and idle-disconnect observation.
src/frontend/src/features/rooms/components/Conference.tsx Stops forwarding and resetting candidate metadata when a conference disconnects.
src/frontend/src/features/rooms/components/Rating.tsx Removes candidate fields from the rating metadata contract.
src/frontend/src/features/rooms/routes/Feedback.tsx Stops reading removed candidate metadata from browser history state.
src/frontend/src/stores/connectionObserver.ts Narrows the observer store to its remaining idle-disconnect modal state.

Reviews (3): Last reviewed commit: "🔥(frontend) drop unused vendored Connec..." | Re-trigger Greptile

@lebaudantoine
lebaudantoine force-pushed the connection-observer branch 2 times, most recently from 02666ab to 3feda42 Compare August 12, 2026 17:47
The vendored ConnectionObserver collected connection data that never
turned out to be useful for debugging.

Remove it to reduce dead code, and re-add a targeted observer later
if a concrete debugging need shows up.
@sonarqubecloud

Copy link
Copy Markdown

@lebaudantoine
lebaudantoine merged commit ac503b3 into main Aug 13, 2026
29 checks passed
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