Skip to content

Conversation

schloerke
Copy link
Collaborator

Before the PR, SessionProxy's make_scope(id) would recursively call up the chain to .make_scope(id). This would hit the root AppSession and call make scope with resolved namespace id. However, this set's the ._parent value of the Proxy session to Root and not the calling session. Ex: A nested module A-B-C's parent session currently is the root session, "", not a proxy session with namespace A-B.

This worked as we would use self._ns (properly resolved) instead of self._parent._ns when needing to construct any namespace values. Since the namespace values is all that we really need and all recursive functions didn't add any intermediate proxy logic, it happen to work until now.

While debugging bookmark handlers, I noticed that the session.bookmark.on_bookmark decorator would not recurse up the parent modules but instead go directly to the root session's bookmark (when calling self._session_proxy._parent.bookmark.on_bookmark(cb): "go to this proxy session's bookmark object and register a callback"). It would start at A-B-C's on_bookmark call and immediately jump to Root session's on_bookmark call, with no in between calls to A-B or A's bookmark object.

To fix this, all SessionProxy object's should have their ._parent session point to the session who created it, not always the root session.

@schloerke schloerke changed the title bug(session): Always have calling session be the parent session when making a scoped session fix(session): Always have calling session be the parent session when making a scoped session Mar 17, 2025
@schloerke schloerke requested a review from Copilot March 17, 2025 16:04
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug with the session scoping mechanism by ensuring that the parent session for a newly created scoped session is the calling session rather than the root session.

  • Updated the implementation of SessionProxy.make_scope to set the calling session as the parent.
  • Added an entry in the CHANGELOG to document the fix.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
CHANGELOG.md Added a changelog entry to document the fix in session scoping.
shiny/session/_session.py Modified make_scope to use the calling session (self) as parent.

@schloerke schloerke marked this pull request as ready for review March 17, 2025 16:06
@schloerke schloerke requested a review from wch March 17, 2025 16:06
@schloerke schloerke enabled auto-merge (squash) March 17, 2025 16:20
@schloerke schloerke merged commit 14b92d4 into main Mar 17, 2025
54 checks passed
@schloerke schloerke deleted the session_parent branch March 17, 2025 16:23
schloerke added a commit that referenced this pull request Mar 17, 2025
…on when making a scoped session (#1923)"

This reverts commit 14b92d4.
schloerke added a commit that referenced this pull request Mar 17, 2025
* main:
  fix: Revert - fix(session): Always have calling session be the parent session when making scoped session (#1924)
  fix(session): Always have calling session be the parent session when making a scoped session (#1923)
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.

2 participants