handle deocrated pages better (#5159)

* handle deocrated pages better

* clear DECORATED_PAGES

* maybe fix the issues with decorated pages?

* print more helpful message

* improve module name thing

Author: adhami3310

reflex/app.py

@@ -35,7 +35,11 @@
 from reflex.app_mixins import AppMixin, LifespanMixin, MiddlewareMixin
 from reflex.compiler import compiler
 from reflex.compiler import utils as compiler_utils
-from reflex.compiler.compiler import ExecutorSafeFunctions, compile_theme
+from reflex.compiler.compiler import (
+    ExecutorSafeFunctions,
+    compile_theme,
+    readable_name_from_component,
+)
 from reflex.components.base.app_wrap import AppWrap
 from reflex.components.base.error_boundary import ErrorBoundary
 from reflex.components.base.fragment import Fragment
@@ -284,6 +288,25 @@ class UnevaluatedPage:
     meta: list[dict[str, str]]
     context: dict[str, Any] | None
 
+    def merged_with(self, other: UnevaluatedPage) -> UnevaluatedPage:
+        """Merge the other page into this one.
+
+        Args:
+            other: The other page to merge with.
+
+        Returns:
+            The merged page.
+        """
+        return dataclasses.replace(
+            self,
+            title=self.title if self.title is not None else other.title,
+            description=self.description
+            if self.description is not None
+            else other.description,
+            on_load=self.on_load if self.on_load is not None else other.on_load,
+            context=self.context if self.context is not None else other.context,
+        )
+
 
 @dataclasses.dataclass()
 class App(MiddlewareMixin, LifespanMixin):
@@ -719,22 +742,37 @@ def add_page(
         # Check if the route given is valid
         verify_route_validity(route)
 
-        if route in self._unevaluated_pages and environment.RELOAD_CONFIG.is_set():
-            # when the app is reloaded(typically for app harness tests), we should maintain
-            # the latest render function of a route.This applies typically to decorated pages
-            # since they are only added when app._compile is called.
-            self._unevaluated_pages.pop(route)
+        unevaluated_page = UnevaluatedPage(
+            component=component,
+            route=route,
+            title=title,
+            description=description,
+            image=image,
+            on_load=on_load,
+            meta=meta,
+            context=context,
+        )
 
         if route in self._unevaluated_pages:
-            route_name = (
-                f"`{route}` or `/`"
-                if route == constants.PageNames.INDEX_ROUTE
-                else f"`{route}`"
-            )
-            raise exceptions.RouteValueError(
-                f"Duplicate page route {route_name} already exists. Make sure you do not have two"
-                f" pages with the same route"
-            )
+            if self._unevaluated_pages[route].component is component:
+                unevaluated_page = unevaluated_page.merged_with(
+                    self._unevaluated_pages[route]
+                )
+                console.warn(
+                    f"Page {route} is being redefined with the same component."
+                )
+            else:
+                route_name = (
+                    f"`{route}` or `/`"
+                    if route == constants.PageNames.INDEX_ROUTE
+                    else f"`{route}`"
+                )
+                existing_component = self._unevaluated_pages[route].component
+                raise exceptions.RouteValueError(
+                    f"Tried to add page {readable_name_from_component(component)} with route {route_name} but "
+                    f"page {readable_name_from_component(existing_component)} with the same route already exists. "
+                    "Make sure you do not have two pages with the same route."
+                )
 
         # Setup dynamic args for the route.
         # this state assignment is only required for tests using the deprecated state kwarg for App
@@ -746,16 +784,7 @@ def add_page(
                 on_load if isinstance(on_load, list) else [on_load]
             )
 
-        self._unevaluated_pages[route] = UnevaluatedPage(
-            component=component,
-            route=route,
-            title=title,
-            description=description,
-            image=image,
-            on_load=on_load,
-            meta=meta,
-            context=context,
-        )
+        self._unevaluated_pages[route] = unevaluated_page
 
     def _compile_page(self, route: str, save_page: bool = True):
         """Compile a page.
@@ -1021,9 +1050,11 @@ def _apply_decorated_pages(self):
 
         This can move back into `compile_` when py39 support is dropped.
         """
+        app_name = get_config().app_name
         # Add the @rx.page decorated pages to collect on_load events.
-        for render, kwargs in DECORATED_PAGES[get_config().app_name]:
+        for render, kwargs in DECORATED_PAGES[app_name]:
             self.add_page(render, **kwargs)
+        DECORATED_PAGES[app_name].clear()
 
     def _validate_var_dependencies(self, state: type[BaseState] | None = None) -> None:
         """Validate the dependencies of the vars in the app.

reflex/compiler/compiler.py

@@ -4,6 +4,7 @@
 
 from collections.abc import Iterable, Sequence
 from datetime import datetime
+from inspect import getmodule
 from pathlib import Path
 from typing import TYPE_CHECKING
 
@@ -676,6 +677,35 @@ def _into_component_once(
     return None
 
 
+def readable_name_from_component(
+    component: Component | ComponentCallable,
+) -> str | None:
+    """Get the readable name of a component.
+
+    Args:
+        component: The component to get the name of.
+
+    Returns:
+        The readable name of the component.
+    """
+    if isinstance(component, Component):
+        return type(component).__name__
+    if isinstance(component, (Var, int, float, str)):
+        return str(component)
+    if isinstance(component, Sequence):
+        return ", ".join(str(c) for c in component)
+    if callable(component):
+        module_name = getattr(component, "__module__", None)
+        if module_name is not None:
+            module = getmodule(component)
+            if module is not None:
+                module_name = module.__name__
+        if module_name is not None:
+            return f"{module_name}.{component.__name__}"
+        return component.__name__
+    return None
+
+
 def into_component(component: Component | ComponentCallable) -> Component:
     """Convert a component to a Component.
 

reflex/config.py

@@ -695,9 +695,6 @@ class EnvironmentVariables:
     # The port to run the backend on.
     REFLEX_BACKEND_PORT: EnvVar[int | None] = env_var(None)
 
-    # Reflex internal env to reload the config.
-    RELOAD_CONFIG: EnvVar[bool] = env_var(False, internal=True)
-
     # If this env var is set to "yes", App.compile will be a no-op
     REFLEX_SKIP_COMPILE: EnvVar[bool] = env_var(False, internal=True)
 

reflex/testing.py

@@ -116,7 +116,6 @@ class AppHarness:
     backend: uvicorn.Server | None = None
     state_manager: StateManager | None = None
     _frontends: list[WebDriver] = dataclasses.field(default_factory=list)
-    _decorated_pages: list = dataclasses.field(default_factory=list)
 
     @classmethod
     def create(
@@ -267,21 +266,13 @@ def _initialize_app(self):
         with chdir(self.app_path):
             # ensure config and app are reloaded when testing different app
             reflex.config.get_config(reload=True)
-            # Save decorated pages before importing the test app module
-            before_decorated_pages = reflex.app.DECORATED_PAGES[self.app_name].copy()
             # Ensure the AppHarness test does not skip State assignment due to running via pytest
             os.environ.pop(reflex.constants.PYTEST_CURRENT_TEST, None)
             os.environ[reflex.constants.APP_HARNESS_FLAG] = "true"
             self.app_module = reflex.utils.prerequisites.get_compiled_app(
                 # Do not reload the module for pre-existing apps (only apps generated from source)
                 reload=self.app_source is not None
             )
-            # Save the pages that were added during testing
-            self._decorated_pages = [
-                p
-                for p in reflex.app.DECORATED_PAGES[self.app_name]
-                if p not in before_decorated_pages
-            ]
         self.app_instance = self.app_module.app
         if self.app_instance and isinstance(
             self.app_instance._state_manager, StateManagerRedis
@@ -500,10 +491,6 @@ def stop(self) -> None:
         if self.frontend_output_thread is not None:
             self.frontend_output_thread.join()
 
-        # Cleanup decorated pages added during testing
-        for page in self._decorated_pages:
-            reflex.app.DECORATED_PAGES[self.app_name].remove(page)
-
     def __exit__(self, *excinfo) -> None:
         """Contextmanager protocol for `stop()`.
 

reflex/utils/prerequisites.py

@@ -371,7 +371,6 @@ def get_app(reload: bool = False) -> ModuleType:
     from reflex.utils import telemetry
 
     try:
-        environment.RELOAD_CONFIG.set(reload)
         config = get_config()
 
         _check_app_name(config)
@@ -384,11 +383,14 @@ def get_app(reload: bool = False) -> ModuleType:
             else config.app_module
         )
         if reload:
+            from reflex.page import DECORATED_PAGES
             from reflex.state import reload_state_module
 
             # Reset rx.State subclasses to avoid conflict when reloading.
             reload_state_module(module=module)
 
+            DECORATED_PAGES.clear()
+
             # Reload the app module.
             importlib.reload(app)
     except Exception as ex:

tests/units/test_app.py

@@ -15,6 +15,7 @@
 import pytest
 import sqlmodel
 from fastapi import FastAPI, UploadFile
+from pytest_mock import MockerFixture
 from starlette_admin.auth import AuthProvider
 from starlette_admin.contrib.sqla.admin import Admin
 from starlette_admin.contrib.sqla.view import ModelView
@@ -49,7 +50,7 @@
     _substate_key,
 )
 from reflex.style import Style
-from reflex.utils import exceptions, format
+from reflex.utils import console, exceptions, format
 from reflex.vars.base import computed_var
 
 from .conftest import chdir
@@ -332,7 +333,28 @@ def index():
 
 
 @pytest.mark.parametrize(
-    "first_page,second_page, route",
+    ("first_page", "second_page", "route"),
+    [
+        (index, index, None),
+        (page1, page1, None),
+    ],
+)
+def test_add_the_same_page(
+    mocker: MockerFixture, app: App, first_page, second_page, route
+):
+    app.add_page(first_page, route=route)
+    mock_object = mocker.Mock()
+    mocker.patch.object(
+        console,
+        "warn",
+        mock_object,
+    )
+    app.add_page(second_page, route="/" + route.strip("/") if route else None)
+    assert mock_object.call_count == 1
+
+
+@pytest.mark.parametrize(
+    ("first_page", "second_page", "route"),
     [
         (lambda: rx.fragment(), lambda: rx.fragment(rx.text("second")), "/"),
         (rx.fragment(rx.text("first")), rx.fragment(rx.text("second")), "/page1"),
@@ -342,11 +364,9 @@ def index():
             "page3",
         ),
         (page1, page2, "page1"),
-        (index, index, None),
-        (page1, page1, None),
     ],
 )
-def test_add_duplicate_page_route_error(app, first_page, second_page, route):
+def test_add_duplicate_page_route_error(app: App, first_page, second_page, route):
     app.add_page(first_page, route=route)
     with pytest.raises(ValueError):
         app.add_page(second_page, route="/" + route.strip("/") if route else None)