Skip to content

Feed the list-view Share modals live metadata - #540

Closed
Maximo-Guk wants to merge 1 commit into
mainfrom
maximo/share-modal-live-metadata
Closed

Maximo-Guk wants to merge 1 commit into
mainfrom
maximo/share-modal-live-metadata

Conversation

@Maximo-Guk

@Maximo-Guk Maximo-Guk commented Sep 21, 2026 •

Copy link
Copy Markdown
Member

WIP

Follow-up to #523.


Devin Review

Opening Share from the workspaces grid or the sidebar snapshotted getMetadata()
once, so an ownerInvitesOnly update while the modal was open never reached it and
the controls the server now refuses stayed visible. Both entry points subscribe
for the modal's lifetime instead, as the editor already does.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions github-actions Bot added the workshop/frontend Changes to the Workshop frontend label Sep 21, 2026

@devin-ai-integration devin-ai-integration 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.

Devin Review found 1 potential issue.

Devin Review

Comment on lines +274 to +283
const subscription = await overseer.subscribeToMetadata((metadata) => {
if (!opened) {
opened = true
setShareTarget({ ...gadget, ...metadata })
return
}
// A functional update so a late delivery after close (prev === null) stays closed.
setShareTarget(prev => prev?.id === gadget.id ? { ...prev, ...metadata } : prev)
})
setShareOverseer({ stub: overseer, subscription })

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Interrupted share opens leak RPC stubs

When the component unmounts during subscribeToMetadata, handleShare resolves after cleanup and leaves both stubs undisposed. The same race exists in onShare.

Learn more

The overseer is created before the subscription RPC begins, but both stubs enter shareOverseer only after that RPC resolves. The unmount cleanup reads shareOverseerRef, so it sees neither stub when navigation unmounts the list during the await. A later state update cannot install resources into an unmounted component, leaving the server-side capabilities alive. The established useWorkspaceOpen lifecycle keeps local stub references and disposes a subscription that resolves after cancellation.

Example: A user clicks Share and immediately navigates away while the subscription RPC is pending. Cleanup sees shareOverseerRef.current === null; when the RPC completes, its subscription and overseer have no remaining disposal path.

Recommended fix: Track cancellation for each share-open attempt and retain both stubs locally until ownership transfers to state. If the component unmounts before resolution, dispose the resolved subscription and overseer instead of calling the state setters. Apply the same lifecycle to onShare.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

@github-actions

Copy link
Copy Markdown

Preview: pr540-maximo-share-4e2aee90

https://pr540-maximo-share-4e2aee90-router.cloudflare-os-previews.workers.dev

Dashboard · deleted when this PR closes

@ask-bonk

ask-bonk Bot commented Sep 21, 2026

Copy link
Copy Markdown

No additional findings beyond the existing inline review comment.

github run

@ndisidore

Copy link
Copy Markdown
Member

What's the motivator here?

@Maximo-Guk
Maximo-Guk marked this pull request as draft September 21, 2026 16:28
@Maximo-Guk

Copy link
Copy Markdown
Member Author

What's the motivator here?

Oops sorry this should be drafted still!

@Maximo-Guk Maximo-Guk closed this Sep 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

workshop/frontend Changes to the Workshop frontend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants