feat: add server-side rendering (SSR)#51
Open
emmapowers wants to merge 25 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds end-to-end server-side rendering (SSR): config, build steps, SSR renderer/orchestrator, session store/cache, client hydration, async build/CLI changes, subprocess helpers, and tests for SSR flows and subprocess management. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Browser Client
participant Server as Web Server (HTTP / WebSocket)
participant Orchestrator as SSROrchestrator
participant SessionStore as SessionStore
participant SSRRenderer as SSRRenderer (Bun)
Client->>Server: GET document (Accept: text/html)
activate Server
Server->>Orchestrator: render_to_response(path, template)
activate Orchestrator
Orchestrator->>SessionStore: create session_id & store(SessionEntry)
Orchestrator->>SSRRenderer: render(serialized_tree) (if renderer available)
SSRRenderer-->>Orchestrator: HTML fragment (or None)
Orchestrator->>Orchestrator: build dehydration payload (sessionId + patches)
Orchestrator-->>Server: full HTML (template with ssr_html + <script>__TRELLIS_SSR__</script>)
deactivate Orchestrator
Server-->>Client: 200 HTML response
deactivate Server
sequenceDiagram
participant Build as Build System
participant BundleStep as SSRBundleBuildStep
participant PreRender as SSRPreRenderStep
participant Renderer as SSRRenderer (Bun)
participant Index as IndexHtmlRenderStep
Build->>BundleStep: run(ctx) (async)
BundleStep->>esbuild: produce ssr.js (node/esm)
BundleStep-->>Build: artifact recorded (metafile)
Build->>PreRender: run(ctx)
PreRender->>Renderer: start() and render(serialized_tree)
Renderer-->>PreRender: HTML fragment
PreRender->>Build: set ctx.template_context (ssr_html, ssr_data)
Build->>Index: run(ctx)
Index->>Index: inject ssr_html & dehydration script into template
Index-->>Build: final HTML artifact
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (7)
tests/py/unit/test_config.py (1)
305-329: Add direct behavioral coverage forssr.This only verifies that
to_json()includes the new field. A small set of focused tests for thessrdefault plus env/CLI overrides would better cover the bool resolution path that actually gates SSR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/py/unit/test_config.py` around lines 305 - 329, Add focused tests around the Config.ssr resolution: create one test that constructs Config(name="myapp", module="main") and asserts config.ssr is the expected default (False/True per current default) and that config.to_json() contains the same boolean; add a second test that uses monkeypatch/env setting to set the SSR environment variable (the env key your code reads for SSR) before building the Config and assert config.ssr reflects the env override; add a third test that simulates the CLI flag (call the CLI/argparse entrypoint or the Config factory used for CLI parsing) to pass the SSR enable/disable flag and assert config.ssr follows the CLI value and that to_json() shows the same value; reference Config, to_json, and the test function test_serializes_all_fields as places to add these small tests.tests/py/unit/test_ssr_bundle_step.py (1)
15-71: Consider extracting common setup into a fixture.Both
test_uses_node_platformandtest_output_name_defaults_to_ssrshare identical mock setup code (MagicMock for ctx, patching get_bin/node_modules_path/get_metafile_path/read_metafile). Extracting this into a pytest fixture would reduce duplication and improve maintainability.♻️ Proposed refactor using a fixture
+import pytest + + +@pytest.fixture +def ssr_build_context(): + """Create a mocked build context for SSR bundle step tests.""" + ctx = MagicMock() + ctx.workspace = MagicMock() + ctx.dist_dir = MagicMock() + ctx.collected = MagicMock() + ctx.collected.modules = [] + ctx.esbuild_args = [] + ctx.manifest = MagicMock() + ctx.manifest.steps = {} + return ctx + + class TestSSRBundleBuildStep: def test_step_name(self) -> None: step = SSRBundleBuildStep() assert step.name == "ssr-bundle-build" - def test_uses_node_platform(self) -> None: + def test_uses_node_platform(self, ssr_build_context: MagicMock) -> None: step = SSRBundleBuildStep() - ctx = MagicMock() - ctx.workspace = MagicMock() - # ... rest of setup ... + ctx = ssr_build_context with ( patch("trellis.bundler.steps.get_bin") as mock_get_bin, # ... rest unchanged ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/py/unit/test_ssr_bundle_step.py` around lines 15 - 71, Both tests (test_uses_node_platform and test_output_name_defaults_to_ssr) duplicate the same setup for SSRBundleBuildStep and the patched helpers; extract that into a pytest fixture (e.g., ssr_build_ctx) that creates the SSRBundleBuildStep instance, constructs ctx with ctx.workspace/ctx.dist_dir/ctx.collected/ctx.manifest and patches get_bin, node_modules_path, get_metafile_path, read_metafile (returning the same fake values), then yields ctx (and optionally the patched mocks) so each test simply accepts the fixture, calls step.run(ctx), and asserts on ctx.exec_in_build_env; update the tests to remove the duplicated setup and use the new fixture name.tests/py/unit/test_handler_resumption.py (1)
61-74: Extract the repeated SSR-session setup into a fixture.The
RenderSession→render_for_ssr()→SessionEntrysetup is duplicated four times, which makes these tests harder to scan than the behavior they're asserting. A small fixture returning a prepared store/entry would keep each case focused on the resumption branch.As per coding guidelines, "Use fixtures for shared test code instead of class members and inheritance; refactor common test code into fixtures and check conftest.py for existing fixtures."
Also applies to: 95-106, 123-135, 157-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/py/unit/test_handler_resumption.py` around lines 61 - 74, Extract the repeated SSR-session setup into a pytest fixture that prepares and returns the SessionStore and SessionEntry (or a tuple of store and the stored key) so tests can reuse it; move the block that calls app_wrapper(noop_component, "light", None), instantiates RenderSession, runs render_for_ssr(session), serializes patches via _serialize_patches, creates SessionEntry(session, deferred_mounts=ssr_result.deferred_mounts, deferred_unmounts=ssr_result.deferred_unmounts, patches=wire_patches) and stores it in a SessionStore(ttl_seconds=30) into a fixture (e.g., prepared_ssr_store) and replace the duplicated blocks in the tests with calls to that fixture; ensure the fixture imports or references app_wrapper, RenderSession, render_for_ssr, _serialize_patches, SessionEntry, SessionStore, and noop_component so tests remain unchanged.src/trellis/platforms/server/platform.py (1)
125-127: Hoist these imports or document why they must stay local.Lazy-loading can be fine here, but this file should say so explicitly. Right now the SSR imports and fallback
loggingimport look accidental.As per coding guidelines, "Place imports at the top of the file unless there is a compelling reason not to; mid-file imports add complexity and should include a comment explaining why".
Also applies to: 162-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/platform.py` around lines 125 - 127, Move the mid-file imports for SessionStore, SSROrchestrator, SSRRenderer (and the fallback logging import around lines 162-165) to the top of the module unless there is a deliberate lazy-load reason; if lazy-loading is required (e.g., to avoid circular imports or heavy startup cost), add a concise explanatory comment immediately above each local import mentioning the reason (e.g., "lazy import to avoid circular dependency with X" or "defer heavy SSR init until runtime") and ensure the import statements remain grouped and marked with noqa if needed; update usages in the module (functions/methods referencing SessionStore, SSROrchestrator, SSRRenderer) accordingly so the imports are clearly documented and consistent.tests/py/unit/test_ssr_orchestrator.py (2)
62-70: This test doesn't actually prove the session was stored.Checking for
"sessionId"in the HTML only proves the dehydration script was emitted. IfSessionStore.store()stops being called, this still passes. Please parse the emitted ID and assert thatSessionStore.pop(session_id)returns aSessionEntry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/py/unit/test_ssr_orchestrator.py` around lines 62 - 70, The test currently only checks that the dehydration script emitted a "sessionId" token; instead parse the actual session_id from the rendered result returned by label_orchestrator.render_to_response (extract the value emitted by the dehydration script), then call the orchestrator's SessionStore.pop(session_id) and assert it returns a SessionEntry (non-None / correct type). In short: extract session_id from result, call SessionStore.pop(session_id) on the SSROrchestrator's session store and assert the returned object is a SessionEntry to verify SessionStore.store() was actually invoked.
72-106: Please add a cache-hit regression test here.Every test in this file does a single cold render. The new production path has separate cache-hit and
invalidate_cache()behavior, and neither is exercised, so cache regressions will slip through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/py/unit/test_ssr_orchestrator.py` around lines 72 - 106, Add a new regression test that exercises both the cache-hit path and cache invalidation by calling SSROrchestrator.render_to_response twice for the same path (e.g., path="/") and asserting the second response is produced from cache (for example the dehydration payload like "sessionId" or "patches" is present and the body content equals the first response), then call SSROrchestrator.invalidate_cache(path) and call render_to_response a third time and assert the response differs from the cached response (e.g., different sessionId or different HTML) to prove invalidation works; reference the SSROrchestrator methods render_to_response and invalidate_cache and reuse the existing html_template and assertion style used in neighboring tests.src/trellis/platforms/server/ssr.py (1)
31-38: Hoist these imports or add a short lazy-import rationale.Both helpers use local imports without explaining why they need to stay there. In
src/trellis/, that reads as accidental rather than intentional.As per coding guidelines, "Place imports at the top of the file unless there is a compelling reason not to; mid-file imports add complexity and should include a comment explaining why".
Also applies to: 167-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/ssr.py` around lines 31 - 38, Move the local import "from importlib.metadata import version as get_package_version" out of the _get_version() function and place it at the top of the module (module-level import), or if the import must remain local, add a short comment above the local import inside _get_version() explaining the intentional lazy-import rationale (e.g., to avoid import-time overhead or optional dependency failures) and keep the existing exception fallback; apply the same change/comment pattern to the other helper that has a mid-file import around lines 167-172 so all non-top-level imports are explicitly justified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/trellis/app/config.py`:
- Line 213: Update the Config class docstring to include the new public boolean
field ssr in the Attributes list so docs match the dataclass surface: locate the
Config dataclass and its docstring (class Config) and add an entry describing
ssr (type bool and brief purpose, e.g., "ssr: Whether server-side rendering is
enabled.") between the existing port and window_size entries, ensuring wording
and formatting match the other attribute lines and no outdated comment text
remains.
In `@src/trellis/bundler/steps.py`:
- Around line 735-752: The SSRBundleBuildStep's should_build currently ignores
the instance's output_name, so switching SSRBundleBuildStep(output_name=...) can
wrongly return SKIP; update should_build to include output_name in the
cache/compare check by comparing self.output_name against a stored value in
StepManifest (e.g., manifest.metadata["output_name"]) or by adding a synthetic
key (like f"output_name:{self.output_name}") to the set passed to
is_rebuild_needed, and return ShouldBuild.BUILD when they differ; apply the same
change to the analogous check referenced at the other occurrence (around the
791-795 area) so both places account for output_name in the rebuild decision.
In `@src/trellis/platforms/common/handler.py`:
- Around line 332-339: When resuming a session, install router callbacks before
running deferred mounts: move or add a call to self._setup_router_callbacks()
(or ensure the same initialization happens) immediately after restoring the
session (after assigning self.session and self.session_id and registering the
session via get_session_registry()) and before calling
execute_deferred_hooks(self.session, ...). Apply the identical change to the
other resume path that also calls execute_deferred_hooks (the similar block
around the second execute_deferred_hooks call) so router callbacks are always
set up prior to replaying deferred on_mount/on_unmount hooks.
In `@src/trellis/platforms/server/routes.py`:
- Around line 37-40: The 404 handler spa_fallback_handler currently routes every
missing path through _ssr_or_static_response, causing assets and API misses to
trigger full SSR; change spa_fallback_handler to first detect real
navigation/document requests (e.g., only handle GET requests whose Accept header
prefers text/html, and whose path is not an API/static asset — skip paths
starting with /api or those with a file extension like .js/.css/.png/.map/etc.).
If the request is a document navigation, call _ssr_or_static_response(request);
otherwise return a normal 404 response (or re-raise the StarletteHTTPException)
so non-document 404s remain cheap and keep proper status codes. Ensure you
reference Request and StarletteHTTPException in the handler's checks.
- Around line 61-72: The responses serving theme-dependent HTML must advertise
the client hint so caches won't serve the wrong variant: add a Vary header with
"Sec-CH-Prefers-Color-Scheme" to the HTMLResponse returned from the SSR path
(where ssr.render_to_response is used and HTMLResponse is returned) and to the
fallback HTMLResponse that returns get_index_html(); update the response
creation (the HTMLResponse instances) to include headers={"Vary":
"Sec-CH-Prefers-Color-Scheme"} so caches and CDNs vary by the detected theme
produced by _detect_theme.
In `@src/trellis/platforms/server/session_store.py`:
- Around line 39-62: The SessionStore currently only removes entries via pop()
and tests call cleanup_expired(), so add a periodic cleanup task tied to the
server/platform lifecycle that calls cleanup_expired() on a configurable
interval; implement this by registering a lifespan/startup handler that starts a
background worker (async task or daemon thread) which invokes
SessionStore.cleanup_expired() every N seconds and stops on shutdown to avoid
leaks, and expose the interval as a config option and ensure the worker respects
the store's _lock and shutdown signals so no race or dangling thread remains.
In `@src/trellis/platforms/server/ssr_cache.py`:
- Around line 18-35: The cache currently stores raw (route, theme) keys
indefinitely; update the SSR cache implementation (constructor __init__,
internal _cache, and methods get/put/invalidate) to bound the key space by
either normalizing routes and/or adding an eviction policy: add a max_entries
(or ttl_seconds) parameter to __init__, change _cache to store metadata (e.g.,
OrderedDict or dict of (html, timestamp)), implement a normalize_route(route)
helper to strip query strings, session IDs and common probe patterns, and modify
put() to insert normalized keys and evict oldest entries when max_entries is
exceeded (or record timestamps and drop expired entries on get/put); keep
invalidate() behavior to clear the store and continue using _lock for
thread-safety.
In `@src/trellis/platforms/server/ssr_renderer.py`:
- Around line 52-57: The child process is started with stdout/stderr set to PIPE
but those streams are never drained, risking a hang when buffers fill; update
the Popen call (where self._process is created using [str(bun),
str(self._bundle_path)]) to either redirect stdout and stderr to
subprocess.DEVNULL or to files, or implement background readers that
continuously read from self._process.stdout and self._process.stderr and forward
logs to your logger (spawn threads or asyncio tasks) so the pipes are actively
drained for the lifetime of the process.
- Around line 46-49: The code currently uses tempfile.mktemp() to create
self._socket_path which is insecure; change to create a private temp directory
via tempfile.mkdtemp() (e.g., set self._socket_dir =
tempfile.mkdtemp(prefix="trellis-ssr-") in __init__) and then set
self._socket_path to a file inside that dir (e.g.,
os.path.join(self._socket_dir, "trellis-ssr.sock")) and export
TRELLIS_SSR_SOCKET accordingly; also initialize self._socket_dir = None in
__init__ and update stop() to unlink the socket and remove the directory
(os.remove/unlink then os.rmdir or shutil.rmtree) to ensure atomic creation and
proper cleanup.
In `@src/trellis/platforms/server/ssr.py`:
- Around line 69-75: Cache currently keys only by path and system_theme causing
wrong HTML to be reused when theme_mode differs; update the cache key usage so
_cache.get and places that write to the cache use a composite key that includes
theme_mode (e.g., (path, system_theme, theme_mode) or a joined string) so the
lookup in the block calling _cache.get(path, system_theme) becomes
_cache.get(path, system_theme, theme_mode) and ensure corresponding cache writes
in _full_render/_app_wrapper use the same composite key; update any helper
methods that construct cache keys to accept theme_mode so _serve_from_cache and
_full_render continue to operate with the new key shape.
- Around line 115-160: _full_render currently never writes to the SSR cache;
after building the template that will be stored in SSRCache you must call
self._cache.put(...) with a template that contains the _SESSION_ID_PLACEHOLDER
so subsequent requests can be resumed. Concretely: after creating
dehydration_data and before returning, construct a cacheable_template by
injecting a placeholder script (e.g. "<script>window.__TRELLIS_SSR__ = " +
_SESSION_ID_PLACEHOLDER + ";</script>") into html_template (replacing the same
<!--ssr-outlet--> and closing </body> slot you use for the real response), then
call self._cache.put(path, cacheable_template) (or whatever cache key SSRCache
expects) and finally return the real response that embeds the actual
session_id/dehydration_data; update _full_render to reference session_id,
dehydration_data, self._cache.put, and _SESSION_ID_PLACEHOLDER as needed.
- Around line 154-160: The dehydration payload generated by
_build_dehydration_data is embedded raw into inline JS (dehydration_script)
which allows payloads containing "</script>" or other sequences to break out and
execute; fix by serializing the payload to a safe JSON string and escaping the
closing script sequence before embedding (e.g., JSON-serialize dehydration_data
and replace "</" with "<\\/") so dehydration_script no longer contains raw
"</script>" or unescaped HTML/JS content when inserted into html_template in
ssr.py.
---
Nitpick comments:
In `@src/trellis/platforms/server/platform.py`:
- Around line 125-127: Move the mid-file imports for SessionStore,
SSROrchestrator, SSRRenderer (and the fallback logging import around lines
162-165) to the top of the module unless there is a deliberate lazy-load reason;
if lazy-loading is required (e.g., to avoid circular imports or heavy startup
cost), add a concise explanatory comment immediately above each local import
mentioning the reason (e.g., "lazy import to avoid circular dependency with X"
or "defer heavy SSR init until runtime") and ensure the import statements remain
grouped and marked with noqa if needed; update usages in the module
(functions/methods referencing SessionStore, SSROrchestrator, SSRRenderer)
accordingly so the imports are clearly documented and consistent.
In `@src/trellis/platforms/server/ssr.py`:
- Around line 31-38: Move the local import "from importlib.metadata import
version as get_package_version" out of the _get_version() function and place it
at the top of the module (module-level import), or if the import must remain
local, add a short comment above the local import inside _get_version()
explaining the intentional lazy-import rationale (e.g., to avoid import-time
overhead or optional dependency failures) and keep the existing exception
fallback; apply the same change/comment pattern to the other helper that has a
mid-file import around lines 167-172 so all non-top-level imports are explicitly
justified.
In `@tests/py/unit/test_config.py`:
- Around line 305-329: Add focused tests around the Config.ssr resolution:
create one test that constructs Config(name="myapp", module="main") and asserts
config.ssr is the expected default (False/True per current default) and that
config.to_json() contains the same boolean; add a second test that uses
monkeypatch/env setting to set the SSR environment variable (the env key your
code reads for SSR) before building the Config and assert config.ssr reflects
the env override; add a third test that simulates the CLI flag (call the
CLI/argparse entrypoint or the Config factory used for CLI parsing) to pass the
SSR enable/disable flag and assert config.ssr follows the CLI value and that
to_json() shows the same value; reference Config, to_json, and the test function
test_serializes_all_fields as places to add these small tests.
In `@tests/py/unit/test_handler_resumption.py`:
- Around line 61-74: Extract the repeated SSR-session setup into a pytest
fixture that prepares and returns the SessionStore and SessionEntry (or a tuple
of store and the stored key) so tests can reuse it; move the block that calls
app_wrapper(noop_component, "light", None), instantiates RenderSession, runs
render_for_ssr(session), serializes patches via _serialize_patches, creates
SessionEntry(session, deferred_mounts=ssr_result.deferred_mounts,
deferred_unmounts=ssr_result.deferred_unmounts, patches=wire_patches) and stores
it in a SessionStore(ttl_seconds=30) into a fixture (e.g., prepared_ssr_store)
and replace the duplicated blocks in the tests with calls to that fixture;
ensure the fixture imports or references app_wrapper, RenderSession,
render_for_ssr, _serialize_patches, SessionEntry, SessionStore, and
noop_component so tests remain unchanged.
In `@tests/py/unit/test_ssr_bundle_step.py`:
- Around line 15-71: Both tests (test_uses_node_platform and
test_output_name_defaults_to_ssr) duplicate the same setup for
SSRBundleBuildStep and the patched helpers; extract that into a pytest fixture
(e.g., ssr_build_ctx) that creates the SSRBundleBuildStep instance, constructs
ctx with ctx.workspace/ctx.dist_dir/ctx.collected/ctx.manifest and patches
get_bin, node_modules_path, get_metafile_path, read_metafile (returning the same
fake values), then yields ctx (and optionally the patched mocks) so each test
simply accepts the fixture, calls step.run(ctx), and asserts on
ctx.exec_in_build_env; update the tests to remove the duplicated setup and use
the new fixture name.
In `@tests/py/unit/test_ssr_orchestrator.py`:
- Around line 62-70: The test currently only checks that the dehydration script
emitted a "sessionId" token; instead parse the actual session_id from the
rendered result returned by label_orchestrator.render_to_response (extract the
value emitted by the dehydration script), then call the orchestrator's
SessionStore.pop(session_id) and assert it returns a SessionEntry (non-None /
correct type). In short: extract session_id from result, call
SessionStore.pop(session_id) on the SSROrchestrator's session store and assert
the returned object is a SessionEntry to verify SessionStore.store() was
actually invoked.
- Around line 72-106: Add a new regression test that exercises both the
cache-hit path and cache invalidation by calling
SSROrchestrator.render_to_response twice for the same path (e.g., path="/") and
asserting the second response is produced from cache (for example the
dehydration payload like "sessionId" or "patches" is present and the body
content equals the first response), then call
SSROrchestrator.invalidate_cache(path) and call render_to_response a third time
and assert the response differs from the cached response (e.g., different
sessionId or different HTML) to prove invalidation works; reference the
SSROrchestrator methods render_to_response and invalidate_cache and reuse the
existing html_template and assertion style used in neighboring tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3a8e337-f4a9-4ab4-b8a2-21ab5e858d1e
📒 Files selected for processing (28)
src/trellis/app/config.pysrc/trellis/bundler/__init__.pysrc/trellis/bundler/steps.pysrc/trellis/core/rendering/__init__.pysrc/trellis/core/rendering/ssr.pysrc/trellis/platforms/common/client/src/types.tssrc/trellis/platforms/common/handler.pysrc/trellis/platforms/common/messages.pysrc/trellis/platforms/server/client/src/TrellisClient.tssrc/trellis/platforms/server/client/src/index.html.j2src/trellis/platforms/server/client/src/main.tsxsrc/trellis/platforms/server/client/src/ssr-entry.tsxsrc/trellis/platforms/server/handler.pysrc/trellis/platforms/server/platform.pysrc/trellis/platforms/server/routes.pysrc/trellis/platforms/server/session_store.pysrc/trellis/platforms/server/ssr.pysrc/trellis/platforms/server/ssr_cache.pysrc/trellis/platforms/server/ssr_renderer.pysrc/trellis/utils/hot_reload.pytests/py/unit/test_build_config.pytests/py/unit/test_config.pytests/py/unit/test_handler_resumption.pytests/py/unit/test_session_store.pytests/py/unit/test_ssr_bundle_step.pytests/py/unit/test_ssr_cache.pytests/py/unit/test_ssr_orchestrator.pytests/py/unit/test_ssr_render.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/trellis/platforms/common/client/src/TrellisRoot.tsx (1)
14-15: Drop the extrauseRootId()wrapper.
TrellisContentis repeating the samerootIdsubscription/null guard thatsrc/trellis/platforms/common/client/src/TreeRenderer.tsxalready does on Lines 17-25. That adds an extra store subscription and leaves a misleading "depending on TrellisContext" abstraction without changing behavior.♻️ Proposed simplification
import React from "react"; import { TrellisClient } from "./TrellisClient"; import { TrellisContext } from "./TrellisContext"; import { TreeRenderer } from "./TreeRenderer"; -import { useRootId } from "./core"; @@ return ( <TrellisContext.Provider value={client}> - <TrellisContent /> + <TreeRenderer /> </TrellisContext.Provider> ); } - -/** - * Inner component that uses hooks depending on TrellisContext. - * Renders nothing until the tree root is available. - */ -function TrellisContent(): React.ReactElement | null { - const rootId = useRootId(); - - if (!rootId) { - return null; - } - - return <TreeRenderer />; -}Also applies to: 39-42, 47-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/common/client/src/TrellisRoot.tsx` around lines 14 - 15, TrellisContent is redundantly calling useRootId and guarding for null when TreeRenderer already subscribes and performs the same rootId/null checks; remove the extra wrapper by deleting the import and any calls to useRootId inside TrellisContent, eliminate the duplicate null-guard logic in TrellisContent, and rely on the existing subscription/guard in TreeRenderer and TrellisContext to provide rootId — ensure TrellisContent simply consumes TrellisContext (or props) and forwards values to the same children/components without creating another store subscription.src/trellis/platforms/server/client/src/main.tsx (1)
22-26: UnusedserverVersionfield inTrellisSSRData.The
serverVersionfield is defined in the interface but not used anywhere in this file. If it's intended for future version compatibility checks or is used in other parts of the system, consider adding a brief comment explaining its purpose. Otherwise, if it's not needed, it could be removed to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/client/src/main.tsx` around lines 22 - 26, The TrellisSSRData interface declares a serverVersion field that's not used in this file; either remove serverVersion from the TrellisSSRData interface to avoid dead/ confusing fields, or if it's intended for cross-process compatibility, add a concise comment above TrellisSSRData explaining its purpose and where it's consumed (e.g., version checks), and ensure any code that should read it references TrellisSSRData.serverVersion accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/trellis/platforms/desktop/client/src/main.tsx`:
- Line 72: The desktop entry removed the loading/reconnect UI by rendering
TrellisRoot with only {client, error}; restore the desktop loading fallback so
the window isn't blank while awaiting the first tree. Update the return in
main.tsx to either (a) render the previous connection/loading component when the
client is initializing or no component tree is present, or (b) pass a
loadingFallback or connection UI prop into TrellisRoot so it shows the desktop
connection UI until the first tree arrives; reference the existing client and
error values and keep ErrorDisplay behavior for real errors. Ensure this change
only affects the desktop entry that uses createRoot() so startup/reconnect shows
the connection UI.
---
Nitpick comments:
In `@src/trellis/platforms/common/client/src/TrellisRoot.tsx`:
- Around line 14-15: TrellisContent is redundantly calling useRootId and
guarding for null when TreeRenderer already subscribes and performs the same
rootId/null checks; remove the extra wrapper by deleting the import and any
calls to useRootId inside TrellisContent, eliminate the duplicate null-guard
logic in TrellisContent, and rely on the existing subscription/guard in
TreeRenderer and TrellisContext to provide rootId — ensure TrellisContent simply
consumes TrellisContext (or props) and forwards values to the same
children/components without creating another store subscription.
In `@src/trellis/platforms/server/client/src/main.tsx`:
- Around line 22-26: The TrellisSSRData interface declares a serverVersion field
that's not used in this file; either remove serverVersion from the
TrellisSSRData interface to avoid dead/ confusing fields, or if it's intended
for cross-process compatibility, add a concise comment above TrellisSSRData
explaining its purpose and where it's consumed (e.g., version checks), and
ensure any code that should read it references TrellisSSRData.serverVersion
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b5b6fac-f359-4dc0-8362-a7c3b64c9701
📒 Files selected for processing (3)
src/trellis/platforms/common/client/src/TrellisRoot.tsxsrc/trellis/platforms/desktop/client/src/main.tsxsrc/trellis/platforms/server/client/src/main.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/trellis/platforms/server/ssr.py (2)
60-65:⚠️ Potential issue | 🟠 MajorInclude
theme_modein the SSR HTML cache key.
App.get_wrapped_top()derives the rendered tree from bothsystem_themeandtheme_mode, but Lines 111-125 only cache bypathandsystem_theme. An explicit light/dark override on the same OS theme can therefore reuse the wrong HTML and hydrate against a different tree.🐛 Minimal fix
- ssr_html = self._get_ssr_html(path, system_theme, session) + ssr_html = self._get_ssr_html(path, system_theme, theme_mode, session) @@ - def _get_ssr_html(self, path: str, system_theme: str, session: RenderSession) -> str: + def _get_ssr_html( + self, + path: str, + system_theme: str, + theme_mode: str | None, + session: RenderSession, + ) -> str: """Get rendered HTML, using the cache when possible.""" - cached = self._cache.get(path, system_theme) + cache_theme = f"{system_theme}:{theme_mode or 'system'}" + cached = self._cache.get(path, cache_theme) @@ - self._cache.put(path, system_theme, ssr_html) + self._cache.put(path, cache_theme, ssr_html)Also applies to: 97-98, 111-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/ssr.py` around lines 60 - 65, The SSR cache key currently uses only path and system_theme while App.get_wrapped_top() depends on both system_theme and theme_mode, so update all cache key constructions and lookups in render_to_response (and the related cache access sites around the html cache logic) to include theme_mode as part of the key (e.g., compose key = (path, system_theme, theme_mode) or string-join them) so that when theme_mode is explicitly set (light/dark/None) the cached HTML matches the rendered tree; ensure the same adjusted key is used for cache set and cache get operations to avoid mismatches and refer to render_to_response and App.get_wrapped_top() when locating the related code.
100-105:⚠️ Potential issue | 🔴 CriticalEscape the dehydration payload before embedding it in inline JS.
Line 104 injects the serialized dehydration data raw into
<script>.wire_patchescan contain user text/prop values, so a payload containing</script>breaks out of the tag and becomes executable HTML/JS.🛡️ Minimal hardening
- dehydration_script = f"<script>window.__TRELLIS_SSR__ = {dehydration_data};</script>" + safe_dehydration_data = ( + dehydration_data.replace("<", "\\u003c") + .replace("\u2028", "\\u2028") + .replace("\u2029", "\\u2029") + ) + dehydration_script = ( + "<script>" + f"window.__TRELLIS_SSR__ = {safe_dehydration_data};" + "</script>" + )Also applies to: 130-148
src/trellis/platforms/server/ssr_cache.py (1)
24-41:⚠️ Potential issue | 🟠 MajorBound the SSR HTML cache.
This cache still grows without limit and only clears on hot reload. Because
src/trellis/platforms/server/ssr.pyfeeds it raw request paths plus theme, long-lived servers can accumulate one permanent entry per probed URL. Add size/TTL eviction or normalize the route key before storing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/ssr_cache.py` around lines 24 - 41, The SSR cache implemented via _cache and methods get/put/invalidate can grow unbounded because keys are raw route+theme; update the cache to bound size and/or apply TTL and normalize route keys before storing: replace the plain dict _cache with a bounded LRU structure (or dict of entries with timestamps) inside the same class, ensure put(route, theme, ssr_html) normalizes route (strip query strings, trailing slashes, and session-specific tokens) and evicts oldest/expired entries when capacity or TTL is exceeded, update get(route, theme) to normalize the incoming route and to check/evict expired entries, and keep invalidate() clearing the underlying store; reference the existing symbols _cache, _lock, get, put, and invalidate when making the changes.src/trellis/platforms/server/session_store.py (1)
34-62:⚠️ Potential issue | 🟠 MajorMake TTL expiry reclaim memory, not just reject resumes.
ttl_secondsis only enforced onpop()andcleanup_expired(). If a client never resumes its SSR session, the entry stays in_entriesindefinitely unless production code schedulescleanup_expired(). Either sweep opportunistically onstore()/pop()or wire a periodic cleanup into the server lifecycle.Verify whether production code actually runs the sweep. Expected result: at least one non-test lifecycle/background call site invoking
SessionStore.cleanup_expired().#!/bin/bash set -euo pipefail echo "### cleanup_expired() call sites in production code" rg -n -C2 '\bcleanup_expired\s*\(' src | sed -n '1,120p' echo echo "### SessionStore construction sites" rg -n -C2 '\bSessionStore\s*\(' src | sed -n '1,120p'
🧹 Nitpick comments (1)
tests/e2e/_run_test_server.py (1)
14-24: Align this helper with Trellis import and component conventions.Use the public
trellisimports here, import widgets viaw, and rename the component toTestApp. That keeps the E2E harness consistent with the rest of the repo and avoids component functions reading like plain helpers.♻️ Suggested cleanup
-from trellis.app.app import App +from trellis import App, component +from trellis import widgets as w from trellis.app.apploader import AppLoader, set_apploader from trellis.app.config import Config -from trellis.core.components.composition import component from trellis.platforms.server.platform import ServerPlatform -from trellis.widgets import Label @@ `@component` -def test_app() -> None: - Label(text="Hello from SSR") +def TestApp() -> None: + w.Label(text="Hello from SSR") @@ - app_instance = App(test_app) + app_instance = App(TestApp) @@ - test_app, + TestApp,As per coding guidelines,
**/*.py: import Trellis core primitives andAppdirectly, and import widgets via thewalias. Based on learnings, any function decorated withcomponentshould be named using PascalCase.Also applies to: 43-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/_run_test_server.py` around lines 14 - 24, Replace the private trellis imports and lower_snake component name with the public-style imports and PascalCase component: import App, AppLoader, set_apploader, and Config from top-level trellis (e.g., "from trellis import App, AppLoader, set_apploader, Config"), import widgets as the alias "w" (e.g., use "w.Label"), and rename the component function test_app to TestApp while keeping the `@component` decorator; update any other similar helper definitions in the file (the block around the second occurrence) so they follow the same public import and PascalCase component conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/check_hydration.js`:
- Around line 61-68: The forEach callbacks using implicit-return arrow functions
cause lint rule lint/suspicious/useIterableCallbackReturn; update the two
callbacks on pageErrors.forEach and warnings.forEach to use block bodies (e.g.,
replace (e) => console.error(" ", e) with (e) => { console.error(" ", e); }
and similarly for (w) => { console.error(" ", w); }) so the callbacks have
explicit block bodies and the lint error is resolved.
In `@tests/e2e/test_ssr_hydration.py`:
- Around line 85-86: The current code skips whenever result.returncode == 2
which hides real failures; change the logic in tests/e2e/test_ssr_hydration.py
around the block that handles the check_hydration.js invocation so that you only
call pytest.skip when stderr clearly indicates a missing optional prerequisite
(e.g., contains substrings like "Playwright not found", "playwright is not
installed", or other explicit missing-playwright messages emitted by
check_hydration.js); otherwise do not skip—raise a failure (use pytest.fail or
assert) and include result.stdout and result.stderr in the failure message so
real SSR/hydration regressions surface. Ensure you reference the result variable
from the subprocess call to check_hydration.js and the script name
check_hydration.js when implementing the conditional.
---
Duplicate comments:
In `@src/trellis/platforms/server/ssr_cache.py`:
- Around line 24-41: The SSR cache implemented via _cache and methods
get/put/invalidate can grow unbounded because keys are raw route+theme; update
the cache to bound size and/or apply TTL and normalize route keys before
storing: replace the plain dict _cache with a bounded LRU structure (or dict of
entries with timestamps) inside the same class, ensure put(route, theme,
ssr_html) normalizes route (strip query strings, trailing slashes, and
session-specific tokens) and evicts oldest/expired entries when capacity or TTL
is exceeded, update get(route, theme) to normalize the incoming route and to
check/evict expired entries, and keep invalidate() clearing the underlying
store; reference the existing symbols _cache, _lock, get, put, and invalidate
when making the changes.
In `@src/trellis/platforms/server/ssr.py`:
- Around line 60-65: The SSR cache key currently uses only path and system_theme
while App.get_wrapped_top() depends on both system_theme and theme_mode, so
update all cache key constructions and lookups in render_to_response (and the
related cache access sites around the html cache logic) to include theme_mode as
part of the key (e.g., compose key = (path, system_theme, theme_mode) or
string-join them) so that when theme_mode is explicitly set (light/dark/None)
the cached HTML matches the rendered tree; ensure the same adjusted key is used
for cache set and cache get operations to avoid mismatches and refer to
render_to_response and App.get_wrapped_top() when locating the related code.
---
Nitpick comments:
In `@tests/e2e/_run_test_server.py`:
- Around line 14-24: Replace the private trellis imports and lower_snake
component name with the public-style imports and PascalCase component: import
App, AppLoader, set_apploader, and Config from top-level trellis (e.g., "from
trellis import App, AppLoader, set_apploader, Config"), import widgets as the
alias "w" (e.g., use "w.Label"), and rename the component function test_app to
TestApp while keeping the `@component` decorator; update any other similar helper
definitions in the file (the block around the second occurrence) so they follow
the same public import and PascalCase component conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02f4f9db-afa5-44b8-b1a5-964884183e1a
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
pyproject.tomlsrc/trellis/platforms/server/session_store.pysrc/trellis/platforms/server/ssr.pysrc/trellis/platforms/server/ssr_cache.pytests/e2e/_run_test_server.pytests/e2e/check_hydration.jstests/e2e/test_ssr_hydration.pytests/py/unit/test_ssr_orchestrator.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/py/unit/test_ssr_orchestrator.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/trellis/cli/run.py (1)
22-28:⚠️ Potential issue | 🟠 MajorForward
ssrintoplatform.run().
Config.ssris resolved, but_build_run_kwargs()never passes it through.ServerPlatform.run()therefore falls back to its defaultssr=True, so--no-ssr/TRELLIS_SERVER_SSRare ignored and build-time/runtime SSR behavior can diverge.Suggested fix
kwargs: dict[str, Any] = { "host": config.host, "port": config.port, "batch_delay": config.batch_delay, "hot_reload": config.hot_reload, + "ssr": config.ssr, "session_ttl": config.session_ttl, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/cli/run.py` around lines 22 - 28, _build_run_kwargs() currently collects host/port/batch_delay/hot_reload/session_ttl but omits Config.ssr, so ServerPlatform.run() uses its default ssr=True and CLI flags/env are ignored; update _build_run_kwargs() to include the ssr key (e.g., "ssr": config.ssr) so the returned kwargs passed into ServerPlatform.run(...) reflect the resolved Config.ssr value.
🧹 Nitpick comments (3)
tests/py/unit/test_cli_run.py (1)
394-403: Coverssrin this helper test too.This test now checks
session_ttl, but not the other new SSR-related kwarg. Addingassert kwargs["ssr"] is Truewould catch the current omission in_build_run_kwargs()instead of letting it slip through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/py/unit/test_cli_run.py` around lines 394 - 403, The test_server_defaults helper doesn't assert the SSR-related kwarg; update the test (test_server_defaults) to check that _build_run_kwargs returns the SSR flag by adding an assertion that kwargs["ssr"] is True so the helper covers the new SSR-related parameter and will catch omissions in _build_run_kwargs.tests/py/unit/test_ssr_orchestrator.py (2)
60-64: Make the session-storage test inspectSessionStore.This only proves the dehydration payload contains a
sessionId. IfSSROrchestrator.render_to_response()stopped callingSessionStore.store(), the test would still pass. Please assert that the emitted ID can actually be retrieved from the store.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/py/unit/test_ssr_orchestrator.py` around lines 60 - 64, Update the test_render_to_response_stores_session to not only assert "sessionId" is in the returned payload but also retrieve that ID (e.g. session_id = result["sessionId"]) and confirm the session was actually stored by calling the orchestrator's SessionStore retrieval API (e.g. label_orchestrator.session_store.get(session_id) or the appropriate retrieval method) and asserting the returned session is not None (and optionally matches expected contents); keep the call to SSROrchestrator.render_to_response unchanged and use the store on label_orchestrator to perform the lookup so the test fails if SSROrchestrator.render_to_response stops calling SessionStore.store().
10-13: Use Trellis public imports in this test.Import
componentfromtrellisand widgets viafrom trellis import widgets as w, then callw.Label(...). That keeps the test aligned with the public API surface.As per coding guidelines, "
**/*.py: Import trellis core primitives and App directly:from trellis import App, component, Stateful" and "Import widgets via thewalias:from trellis import widgets as w, then usew.Button,w.Label, etc."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/py/unit/test_ssr_orchestrator.py` around lines 10 - 13, Update the test to use Trellis public imports: replace direct imports of component and Label with public API imports (import component from trellis and import widgets as w) and update usages to call w.Label(...) instead of Label; keep other imports like CompositionComponent, SessionStore, and SSROrchestrator as needed but ensure any core primitives follow the guideline (e.g., use from trellis import component and from trellis import widgets as w).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/trellis/app/config.py`:
- Around line 121-126: The _SESSION_TTL ConfigVar must reject non-positive
values during resolution: add a validation step to the ConfigVar resolution
logic (or attach a validator to the _SESSION_TTL instance) that checks the
resolved value and raises an error if <= 0; specifically update
ConfigVar.resolve (or provide a validator callable for _SESSION_TTL) to detect
values <= 0 and raise a ValueError (or ConfigError) with a clear message so
session_ttl cannot be zero or negative before being used by the session cleanup
loop in platform.py.
In `@tests/py/unit/test_ssr_orchestrator.py`:
- Around line 90-113: The test currently uses label_orchestrator with
ssr_renderer=None so SSROrchestrator._get_ssr_html never populates the cache;
change the fixture or test to provide a stub/mock renderer (implementing the
same interface used by SSROrchestrator, e.g., a simple callable that returns
HTML and is instrumented to count calls) and pass it into the SSROrchestrator
used by the test, then call render_to_response twice with the same
path/system_theme and assert the stub renderer was invoked exactly once while
both returned HTML strings still contain "window.__TRELLIS_SSR__" and produce
different session IDs as before. Ensure you reference
SSROrchestrator._get_ssr_html and render_to_response when locating where to
inject the stub and where to assert the call count.
---
Outside diff comments:
In `@src/trellis/cli/run.py`:
- Around line 22-28: _build_run_kwargs() currently collects
host/port/batch_delay/hot_reload/session_ttl but omits Config.ssr, so
ServerPlatform.run() uses its default ssr=True and CLI flags/env are ignored;
update _build_run_kwargs() to include the ssr key (e.g., "ssr": config.ssr) so
the returned kwargs passed into ServerPlatform.run(...) reflect the resolved
Config.ssr value.
---
Nitpick comments:
In `@tests/py/unit/test_cli_run.py`:
- Around line 394-403: The test_server_defaults helper doesn't assert the
SSR-related kwarg; update the test (test_server_defaults) to check that
_build_run_kwargs returns the SSR flag by adding an assertion that kwargs["ssr"]
is True so the helper covers the new SSR-related parameter and will catch
omissions in _build_run_kwargs.
In `@tests/py/unit/test_ssr_orchestrator.py`:
- Around line 60-64: Update the test_render_to_response_stores_session to not
only assert "sessionId" is in the returned payload but also retrieve that ID
(e.g. session_id = result["sessionId"]) and confirm the session was actually
stored by calling the orchestrator's SessionStore retrieval API (e.g.
label_orchestrator.session_store.get(session_id) or the appropriate retrieval
method) and asserting the returned session is not None (and optionally matches
expected contents); keep the call to SSROrchestrator.render_to_response
unchanged and use the store on label_orchestrator to perform the lookup so the
test fails if SSROrchestrator.render_to_response stops calling
SessionStore.store().
- Around line 10-13: Update the test to use Trellis public imports: replace
direct imports of component and Label with public API imports (import component
from trellis and import widgets as w) and update usages to call w.Label(...)
instead of Label; keep other imports like CompositionComponent, SessionStore,
and SSROrchestrator as needed but ensure any core primitives follow the
guideline (e.g., use from trellis import component and from trellis import
widgets as w).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd01c05c-2e45-426a-ba7c-39bd713d1d5a
📒 Files selected for processing (7)
src/trellis/app/config.pysrc/trellis/cli/run.pysrc/trellis/platforms/server/platform.pysrc/trellis/platforms/server/session_store.pytests/py/unit/test_cli_run.pytests/py/unit/test_config.pytests/py/unit/test_ssr_orchestrator.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/py/unit/test_config.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/trellis/platforms/desktop/platform.py (1)
137-145:⚠️ Potential issue | 🟠 MajorGate the desktop SSR steps on the new
ssrconfig flag.The PR introduces
ssras a feature toggle, but this build config always addsSSRBundleBuildStep()andSSRPreRenderStep(). Withssr=False, desktop will still pre-render and embed SSR payloads instead of falling back to CSR.🪄 Suggested direction
return BuildConfig( entry_point=entry_point, - steps=[ - PackageInstallStep(), - RegistryGenerationStep(), - BundleBuildStep(output_name="bundle"), - SSRBundleBuildStep(), - StaticFileCopyStep(), - IconAssetStep(icon_path=config.icon, include_icns=True), - SSRPreRenderStep(), - IndexHtmlRenderStep(template_path, {"title": config.title}), - ], + steps=[ + PackageInstallStep(), + RegistryGenerationStep(), + BundleBuildStep(output_name="bundle"), + *([SSRBundleBuildStep()] if config.ssr else []), + StaticFileCopyStep(), + IconAssetStep(icon_path=config.icon, include_icns=True), + *([SSRPreRenderStep()] if config.ssr else []), + IndexHtmlRenderStep(template_path, {"title": config.title}), + ], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/desktop/platform.py` around lines 137 - 145, The build pipeline always includes SSRBundleBuildStep and SSRPreRenderStep regardless of the new feature toggle; update the code that constructs the steps list (where PackageInstallStep, RegistryGenerationStep, BundleBuildStep, SSRBundleBuildStep, SSRPreRenderStep, etc. are created) to conditionally append SSRBundleBuildStep() and SSRPreRenderStep() only when config.ssr is truthy (e.g., if config.ssr: steps.extend([SSRBundleBuildStep(), SSRPreRenderStep()]); otherwise omit them) so that when ssr=False the desktop build falls back to CSR.
♻️ Duplicate comments (2)
src/trellis/bundler/steps.py (1)
742-752:⚠️ Potential issue | 🟠 MajorInclude
output_namein SSR bundle staleness tracking.
output_nameis part of the public API and changes the emitted filename, butshould_build()still only compares old inputs/outputs. ChangingSSRBundleBuildStep(output_name=...)against an existing workspace can incorrectly returnSKIPand leave the old bundle name in the manifest/dist state.♻️ Suggested fix
def should_build( self, ctx: BuildContext, step_manifest: StepManifest | None ) -> ShouldBuild | None: """Check if SSR bundle needs rebuilding based on source/output mtimes.""" if step_manifest is None: return ShouldBuild.BUILD + if step_manifest.metadata.get("output_name") != self.output_name: + return ShouldBuild.BUILD if not step_manifest.source_paths or not step_manifest.dest_files: return ShouldBuild.BUILD if is_rebuild_needed(step_manifest.source_paths, step_manifest.dest_files): return ShouldBuild.BUILD return ShouldBuild.SKIP @@ # Populate step manifest from metafile metafile = read_metafile(ctx.workspace) step_manifest = ctx.manifest.steps.setdefault(self.name, StepManifest()) + step_manifest.source_paths.clear() + step_manifest.dest_files.clear() step_manifest.source_paths.update(metafile.inputs) step_manifest.dest_files.update(metafile.outputs) + step_manifest.metadata["output_name"] = self.output_nameAlso applies to: 791-795
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/bundler/steps.py` around lines 742 - 752, The should_build method (used by SSRBundleBuildStep) currently only checks StepManifest.source_paths and dest_files via is_rebuild_needed and ignores the public API output_name; when output_name changes the emitted filename should count as a change so should_build returns BUILD rather than SKIP. Update should_build to compare the current step's output_name (from SSRBundleBuildStep or its config) against the saved value in StepManifest (e.g., manifest.output_name or add a field) and return ShouldBuild.BUILD when they differ; apply the same change to the other should_build variant referenced in the diff so both code paths consider output_name when deciding rebuilds.src/trellis/platforms/desktop/client/src/main.tsx (1)
89-103:⚠️ Potential issue | 🟠 MajorKeep a desktop loading fallback when SSR data is absent.
The SSR path is fine, but the CSR fallback still renders
TrellisRoot, andTrellisRootonly shows an error or the tree. Whenwindow.__TRELLIS_SSR__is missing, startup/reconnect falls back to a blank window until the first tree arrives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/desktop/client/src/main.tsx` around lines 89 - 103, When ssrData is absent the CSR path currently mounts <App/> which results in TrellisRoot showing only an error or the tree and leaving a blank window until the first tree arrives; update the CSR branch so the initial UI shows a desktop loading fallback: either render TrellisRoot (the same component used for SSR) with a loading prop/flag when ssrData is falsy, or modify TrellisRoot to detect an empty store and render a desktop loading placeholder when neither error nor tree data exist. Locate the CSR code around ssrData, createRoot, root.render and the TrellisRoot/App components and add the loading fallback behavior there.
🧹 Nitpick comments (1)
src/trellis/platforms/desktop/client/src/index.html.j2 (1)
53-61: Consider escaping inline JSON to prevent accidental breakage.Direct interpolation of JSON objects into
<script>context (lines 57-58) can break if any string value contains</script>. While the comment notes this is trusted build-time content, a defensive approach would prevent subtle breakage if component data ever includes that sequence.🛡️ Safer pattern using JSON.parse
<script> // Set dehydration data for the active theme (function() { var isDark = window.matchMedia('(prefers-color-scheme: dark)').matches; - var lightData = {{ ssr_light_data | safe }}; - var darkData = {{ ssr_dark_data | safe }}; + var lightData = JSON.parse('{{ ssr_light_data | tojson | safe }}'); + var darkData = JSON.parse('{{ ssr_dark_data | tojson | safe }}'); window.__TRELLIS_SSR__ = isDark ? darkData : lightData; })(); </script>Alternatively, the Python side can pre-escape
</as<\/before templating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/desktop/client/src/index.html.j2` around lines 53 - 61, The inline JSON injection for ssr_light_data and ssr_dark_data can break the script if values contain "</script>"; update the template to render those payloads as escaped JSON strings and parse them in the IIFE instead of directly interpolating objects: produce safe stringified/escaped versions of ssr_light_data and ssr_dark_data on the server (or escape "</" as "<\/"), then in the script use JSON.parse on the injected string tokens and assign the result to window.__TRELLIS_SSR__ (references: ssr_light_data, ssr_dark_data, window.__TRELLIS_SSR__).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/trellis/bundler/steps.py`:
- Around line 834-836: The call to renderer.start() is currently outside the try
block so if SSRRenderer.start() raises after spawning the Bun sidecar the
finally cleanup never runs; move the renderer.start() invocation inside the try
block that follows its construction (i.e., after creating
SSRRenderer(ssr_bundle) and before the existing try body) so any exceptions from
SSRRenderer.start() still trigger the existing finally cleanup that shuts down
the child process.
- Around line 823-826: SSRPreRenderStep currently hardcodes ctx.dist_dir /
"ssr.js", causing pre-render to skip when SSRBundleBuildStep used a non-default
output_name; change the flow so SSRBundleBuildStep.run() stores the actual
bundle filename into the build context (e.g. set ctx["ssr_bundle_name"] or
ctx.ssr_bundle_path when it produces the bundle) and ensure
SSRBundleBuildStep.should_build() restores or preserves that context value when
the step is skipped, then update SSRPreRenderStep to resolve the bundle path
from that context value instead of "ssr.js" (use ctx.dist_dir /
ctx.ssr_bundle_name or the equivalent key you add).
In `@src/trellis/platforms/common/ssr_utils.py`:
- Around line 52-54: The current manual JSON concat in ssr_utils.py (using
data_json, patches_json and _PATCHES_KEY) embeds raw JSON into HTML and can be
broken out of by characters like </script>, <, &, \u2028, or \u2029; update the
return logic to escape the dehydration payload before embedding: produce
data_json and patches_json via json.dumps as now, then run a replacer that
escapes problematic sequences (at minimum replace "</script>" with
"<\\/script>", replace "<" and "&" with their safe escaped forms, and replace
\u2028 and \u2029) before concatenating with _PATCHES_KEY, so the returned
string is safe to embed in a <script> tag. Ensure you apply the same escaping to
both data_json and patches_json and keep using the _PATCHES_KEY insertion logic.
In `@src/trellis/platforms/desktop/client/src/index.html.j2`:
- Around line 43-50: The block that queries and removes the template elements
(the variables "light" and "dark" and the subsequent if (light) light.remove();
if (dark) dark.remove();) is unreachable after assigning root.innerHTML =
template.innerHTML because the templates were children of `#root` and are already
removed; remove the dead code referencing document.getElementById('ssr-light')
and document.getElementById('ssr-dark') and their conditional .remove() calls
(or, alternatively, move any needed cleanup to before root.innerHTML is set) so
only live, necessary DOM manipulation remains in the function that handles
"template" and "root".
In `@src/trellis/utils/subprocess.py`:
- Around line 96-103: In _stop_posix, avoid signaling a recycled PID by
returning early if the child has already been reaped: check process.returncode
and if it is not None, skip the os.getpgid/os.killpg logic and return; otherwise
proceed to call os.getpgid(process.pid), os.killpg(pgid, signal.SIGTERM) and
process.wait(timeout=timeout). This prevents sending signals to an unrelated
process group when the PID was recycled.
- Around line 84-90: The _set_pdeathsig function currently hardcodes
ctypes.CDLL("libc.so.6"), which fails on non-glibc Linux (e.g., musl/Alpine);
update it to locate the C library via ctypes.util.find_library("c") and pass
that name to ctypes.CDLL, falling back to ctypes.CDLL(None) if find_library
returns None, and keep use_errno=True and the existing prctl invocation so the
function works across glibc and musl systems.
- Around line 129-180: The Win32 calls (kernel32.SetInformationJobObject,
kernel32.AssignProcessToJobObject, kernel32.OpenProcess, kernel32.CloseHandle)
must be properly typed and their return values checked before storing
process._job_handle; set restype/argtypes (e.g., restype=ctypes.wintypes.BOOL
for SetInformationJobObject/AssignProcessToJobObject and
restype=ctypes.wintypes.HANDLE for OpenProcess), inspect the returns and call
CloseHandle on any handles you opened if a subsequent step fails, and only
assign process._job_handle = job when AssignProcessToJobObject succeeds; also
surface or log GetLastError when a call fails to aid debugging.
---
Outside diff comments:
In `@src/trellis/platforms/desktop/platform.py`:
- Around line 137-145: The build pipeline always includes SSRBundleBuildStep and
SSRPreRenderStep regardless of the new feature toggle; update the code that
constructs the steps list (where PackageInstallStep, RegistryGenerationStep,
BundleBuildStep, SSRBundleBuildStep, SSRPreRenderStep, etc. are created) to
conditionally append SSRBundleBuildStep() and SSRPreRenderStep() only when
config.ssr is truthy (e.g., if config.ssr: steps.extend([SSRBundleBuildStep(),
SSRPreRenderStep()]); otherwise omit them) so that when ssr=False the desktop
build falls back to CSR.
---
Duplicate comments:
In `@src/trellis/bundler/steps.py`:
- Around line 742-752: The should_build method (used by SSRBundleBuildStep)
currently only checks StepManifest.source_paths and dest_files via
is_rebuild_needed and ignores the public API output_name; when output_name
changes the emitted filename should count as a change so should_build returns
BUILD rather than SKIP. Update should_build to compare the current step's
output_name (from SSRBundleBuildStep or its config) against the saved value in
StepManifest (e.g., manifest.output_name or add a field) and return
ShouldBuild.BUILD when they differ; apply the same change to the other
should_build variant referenced in the diff so both code paths consider
output_name when deciding rebuilds.
In `@src/trellis/platforms/desktop/client/src/main.tsx`:
- Around line 89-103: When ssrData is absent the CSR path currently mounts
<App/> which results in TrellisRoot showing only an error or the tree and
leaving a blank window until the first tree arrives; update the CSR branch so
the initial UI shows a desktop loading fallback: either render TrellisRoot (the
same component used for SSR) with a loading prop/flag when ssrData is falsy, or
modify TrellisRoot to detect an empty store and render a desktop loading
placeholder when neither error nor tree data exist. Locate the CSR code around
ssrData, createRoot, root.render and the TrellisRoot/App components and add the
loading fallback behavior there.
---
Nitpick comments:
In `@src/trellis/platforms/desktop/client/src/index.html.j2`:
- Around line 53-61: The inline JSON injection for ssr_light_data and
ssr_dark_data can break the script if values contain "</script>"; update the
template to render those payloads as escaped JSON strings and parse them in the
IIFE instead of directly interpolating objects: produce safe stringified/escaped
versions of ssr_light_data and ssr_dark_data on the server (or escape "</" as
"<\/"), then in the script use JSON.parse on the injected string tokens and
assign the result to window.__TRELLIS_SSR__ (references: ssr_light_data,
ssr_dark_data, window.__TRELLIS_SSR__).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e70ceaf-1aca-452d-a2d7-c53b494c0275
📒 Files selected for processing (13)
src/trellis/bundler/__init__.pysrc/trellis/bundler/steps.pysrc/trellis/platforms/common/ssr_utils.pysrc/trellis/platforms/desktop/client/src/index.html.j2src/trellis/platforms/desktop/client/src/main.tsxsrc/trellis/platforms/desktop/platform.pysrc/trellis/platforms/server/ssr.pysrc/trellis/platforms/server/ssr_renderer.pysrc/trellis/utils/subprocess.pytests/py/unit/test_build_config.pytests/py/unit/test_ssr_pre_render_step.pytests/py/unit/test_ssr_utils.pytests/py/unit/test_subprocess_utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/trellis/platforms/server/ssr.py
Render the component tree to HTML on the server and ship it in the initial HTTP response so users see content immediately instead of a blank page while the WebSocket connects. Key changes: - render_for_ssr() defers mount hooks; execute_deferred_hooks() replays them when the WebSocket connects and resumes the session - SessionStore bridges SSR HTTP responses and WebSocket resumption with a 30-second TTL - SSRBundleBuildStep produces a Node-targeted ESM bundle (ssr.js) from a Bun SSR worker entry point that calls React renderToString() - SSRRenderer manages the Bun sidecar subprocess over a Unix socket - SSROrchestrator coordinates rendering, session storage, dehydration data injection, and per-route+theme HTML caching - Client detects window.__TRELLIS_SSR__, pre-populates the store, and uses hydrateRoot() instead of createRoot() - ssr config option (default true) controls the feature; graceful fallback to CSR when the renderer is unavailable
Show a blank page while waiting for the component tree instead of the connection status bar (state/session ID/version). With SSR the user already sees content; without SSR a blank page is preferable to a status bar that flashes for 200ms. ErrorDisplay is kept for catastrophic failures. Browser platform (TrellisApp) is unchanged since Pyodide loading takes seconds and the spinner is warranted there.
Cache renderToString output separately from dehydration data since element IDs are non-deterministic across sessions. Increase session TTL from 30s to 120s. Add Playwright-based e2e test for hydration mismatch detection.
Add session_ttl (default 300s) to Config, wired through to SessionStore. Periodic asyncio task cleans up expired sessions. Configurable via trellis_config.py or TRELLIS_SERVER_SESSION_TTL env.
Ensures Bun SSR sidecar dies when parent Python process dies using platform-specific mechanisms (prctl on Linux, process groups on macOS, Job Objects on Windows). SSRRenderer now uses the shared helpers.
Move build_dehydration_data to trellis.platforms.common.ssr_utils so both server (runtime) and desktop (build-time) SSR can reuse it. session_id is now optional (None for desktop).
Pre-render both light and dark themes at build time via SSRPreRenderStep. The desktop index.html uses <template> elements for each theme variant, selecting the correct one before first paint based on system preference. main.tsx gains hydration support (hydrateRoot + store.applyPatches).
- Escape </script> in dehydration JSON (replace < with \u003c) - Replace deprecated tempfile.mktemp with mkdtemp for socket paths - Redirect sidecar stdout/stderr to DEVNULL to prevent pipe buffer hangs - Add threading.Lock to SSRRenderer for concurrent render/restart safety - Include theme_mode in SSR cache key to avoid cross-theme collisions - Bound SSRCache to 100 entries with FIFO eviction - Filter non-document 404s from SPA fallback (check Accept: text/html) - Add Vary: Sec-CH-Prefers-Color-Scheme header to SSR responses - Install router callbacks before replaying deferred mount hooks - Validate session_ttl as positive integer - Document ssr field in Config docstring - Include output_name in SSRBundleBuildStep cache key - Improve hydration test exit code handling (only skip on missing Playwright) - Add cache reuse test with stub renderer
- subprocess.py: use find_library("c") for musl/Alpine portability,
short-circuit on already-reaped processes, type Win32 kernel32 calls
with restype and check return values
- steps.py: accept output_name parameter in SSRPreRenderStep, move
renderer.start() inside try block for cleanup on failure
- index.html.j2: remove dead template cleanup code (innerHTML replaces
all children)
- check_hydration.js: use block bodies for forEach callbacks (Biome lint)
- test_subprocess_utils.py: mock kernel32 in Windows tests, add
already-reaped process test, fix returncode on mock processes
render_for_ssr() now temporarily sets the render session on the current context so component creation (which checks get_render_session()) works correctly after the session management changes on main.
React's hydrateRoot requires useSyncExternalStore to have a third getServerSnapshot argument. Without it, hydration fails and falls back to full client rendering.
Pre-render the app at bundle time for both light and dark themes (reusing the desktop SSR pipeline) so browser apps show real UI immediately instead of a blank page while Pyodide loads. A theme-aware loading overlay appears from the first paint (baked into the HTML template) and transitions to React-managed status updates once hydration completes, then dismisses when connected.
All SSR HTML uses CSS variables for colors, so light and dark renders produce identical markup. Render once and inline the HTML directly in #root — no <template> elements or theme-selection scripts needed. The early theme detection script sets data-theme before first paint, and CSS variables resolve to the correct colors automatically.
emmapowers
commented
Mar 18, 2026
- Fix subprocess.py type errors: use Popen[Any] since callers pass arbitrary kwargs, and suppress wintypes import on non-Windows - Remove unused _background_tasks and _render_task from MessageHandler (task tracking moved to session on main, these were stale SSR branch artifacts from conflict resolution)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/trellis/platforms/common/handler.py (1)
387-400:⚠️ Potential issue | 🟠 MajorReinstall router callbacks after the normal initial render.
The SSR early-return branch calls
_setup_router_callbacks(), but the fresh-session branch now returns directly afterrender(self.session). That leaves CSR/ssr=falsesessions without_on_navigate/_on_go_back/_on_go_forward, so server-driven navigation stops updating browser history.🩹 Minimal fix
logger.debug( "Initial render complete, sending PatchMessage (%d elements)", element_count ) + self._setup_router_callbacks() return PatchMessage(patches=wire_patches)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/common/handler.py` around lines 387 - 400, The initial-render branch skips router setup but the normal fresh-session branch returns immediately after computing wire_patches, leaving CSR sessions without navigation callbacks; after calling render(self.session) and before returning PatchMessage(patches=wire_patches) invoke self._setup_router_callbacks() so that _on_navigate/_on_go_back/_on_go_forward are installed for non-SSR sessions (ensure this happens after _serialize_patches(render_patches, self.session) and before returning the PatchMessage).
♻️ Duplicate comments (4)
tests/e2e/test_ssr_hydration.py (1)
85-89:⚠️ Potential issue | 🟠 MajorNarrow missing-Playwright detection to explicit Playwright errors.
Line 87 treats any
"Cannot find module"as skippable. That can mask real harness regressions unrelated to Playwright.Suggested fix
if result.returncode == 2: stderr = result.stderr.strip() - if "Playwright not installed" in stderr or "Cannot find module" in stderr: + lower = stderr.lower() + missing_playwright = ( + "playwright not installed" in lower + or "playwright is not installed" in lower + or "cannot find module 'playwright'" in lower + or 'cannot find module "@playwright/test"' in lower + ) + if missing_playwright: pytest.skip(f"Playwright not available: {stderr}") pytest.fail(f"Hydration check script error:\nstdout: {result.stdout}\nstderr: {stderr}")#!/bin/bash set -euo pipefail # Verify what check_hydration.js actually treats as missing-Playwright conditions. nl -ba tests/e2e/check_hydration.js | sed -n '1,260p' rg -n "playwright|Cannot find module|process.exit|stderr|returncode" tests/e2e/check_hydration.js -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/test_ssr_hydration.py` around lines 85 - 89, The current skip logic in the result.returncode == 2 branch is too broad because it treats any "Cannot find module" as Playwright missing; update the condition in tests/e2e/test_ssr_hydration.py (the block handling result.returncode == 2 and the stderr variable) to only skip when stderr explicitly indicates Playwright is missing — e.g., stderr contains "Playwright not installed" or matches "Cannot find module.*playwright" (use a regex/search rather than a plain "Cannot find module" substring check); leave other errors to hit pytest.fail so real harness regressions are not masked.src/trellis/platforms/server/routes.py (2)
27-35:⚠️ Potential issue | 🟡 MinorTighten the SPA fallback gate.
Checking only
Accept: text/htmlstill routesPOST404s and browser visits to/api/...or obvious asset URLs through the SPA. Those misses can come back as200HTML and still trigger SSR instead of preserving the real 404.Also applies to: 49-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/routes.py` around lines 27 - 35, The SPA fallback gate in _is_document_request should be tightened: update the _is_document_request(request: Request) function (and the equivalent logic at the other occurrence) to only return True for navigational requests by requiring request.method to be GET or HEAD, ensuring "text/html" is in request.headers.get("accept", ""), and excluding obvious API or asset requests (e.g. request.url.path startswith "/api" or the path has a file extension like ".js", ".css", ".png" — detect by checking for a '.' after the last '/' in the path). Implement these checks together so only true browser navigations trigger the SPA/SSR fallback.
69-87:⚠️ Potential issue | 🟠 MajorDon't run the synchronous SSR path on the ASGI event loop.
index()andspa_fallback_handler()call_ssr_or_static_response()directly, and that path does a fullrender_for_ssr()plus a blockinghttpx.Client.post()to the Bun sidecar. One slow render will stall unrelated HTTP/WebSocket work until it finishes; offload it to a worker thread or make the orchestrator/renderer async end-to-end.Also applies to: 103-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/routes.py` around lines 69 - 87, The synchronous SSR path in _ssr_or_static_response (used by index() and spa_fallback_handler) blocks the ASGI event loop because ssr.render_to_response (which leads to render_for_ssr and a blocking httpx.Client.post to the Bun sidecar) runs on the main thread; change this by offloading the blocking work to a thread pool or converting the orchestrator/renderer to async: either call the blocking ssr.render_to_response via loop.run_in_executor (or asyncio.to_thread) and await it, or implement/replace ssr.render_to_response with an async render_to_response_async that uses an async HTTP client, and update _get_ssr_orchestrator/_ssr_or_static_response to await the async path; ensure headers and exception handling remain the same.src/trellis/platforms/server/ssr.py (1)
73-91:⚠️ Potential issue | 🟠 MajorDon't advertise SSR when no SSR HTML exists.
_get_ssr_html()returns""when the Bun sidecar is absent or unhealthy, butrender_to_response()still stores the session and injectswindow.__TRELLIS_SSR__. The client then tries to hydrate markup that was never sent, so the "renderer unavailable" path no longer falls back to plain CSR.🩹 Minimal fix
- # Store session for WebSocket resumption - entry = SessionEntry( - session=session, - deferred_mounts=ssr_result.deferred_mounts, - deferred_unmounts=ssr_result.deferred_unmounts, - patches=wire_patches, - ) - self._session_store.store(session_id, entry) - # Get rendered HTML — from cache or Bun sidecar ssr_html = self._get_ssr_html(path, system_theme, theme_mode, session) + if ssr_html is None: + raise RuntimeError("SSR renderer unavailable") # Build dehydration data (always fresh — element IDs are per-session) dehydration_data = build_dehydration_data(session_id, wire_patches) + + entry = SessionEntry( + session=session, + deferred_mounts=ssr_result.deferred_mounts, + deferred_unmounts=ssr_result.deferred_unmounts, + patches=wire_patches, + ) + self._session_store.store(session_id, entry) @@ - ) -> str: + ) -> str | None: """Get rendered HTML, using the cache when possible.""" @@ - ssr_html = "" + ssr_html: str | None = None if self._ssr_renderer is not None and self._ssr_renderer.is_available: assert session.root_element is not None tree = serialize_element(session.root_element, session) rendered = self._ssr_renderer.render(tree) if rendered is not None: - ssr_html = rendered - self._cache.put(path, cache_key, ssr_html) + self._cache.put(path, cache_key, rendered) + ssr_html = rendered return ssr_htmlAlso applies to: 97-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/ssr.py` around lines 73 - 91, The code currently stores sessions and injects the dehydration script even when _get_ssr_html(...) returns an empty string (no SSR available), causing the client to attempt hydration with no SSR markup; change render logic so you first call ssr_html = self._get_ssr_html(...), then only create and store SessionEntry via self._session_store.store(session_id, entry), build_dehydration_data(...), and inject the dehydration_script when ssr_html is non-empty; if ssr_html is empty, skip storing the session and skip adding window.__TRELLIS_SSR__ (leave the template without the SSR outlet/dehydration injection) — apply the same guard to the second identical block around build_dehydration_data/_session_store usage.
🧹 Nitpick comments (7)
src/trellis/utils/subprocess.py (1)
241-242: Addtype: ignorecomment for consistency with line 138.The static analysis hint about
wintypesis a type-checker limitation on non-Windows systems. Line 138 already has# type: ignore[attr-defined]for the same pattern; this line should match for consistency and to suppress the CI warning.♻️ Proposed fix
- kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) # type: ignore[attr-defined] - kernel32.CloseHandle.restype = ctypes.wintypes.BOOL + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) # type: ignore[attr-defined] + kernel32.CloseHandle.restype = ctypes.wintypes.BOOL # type: ignore[attr-defined]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/utils/subprocess.py` around lines 241 - 242, The assignment to kernel32.CloseHandle.restype triggers the same type-checker warning as earlier; update the CloseHandle.restype line in subprocess.py (the kernel32 = ctypes.WinDLL(...) / kernel32.CloseHandle.restype = ...) to include the same ignore comment used on line 138 (append # type: ignore[attr-defined]) so the static analyzer is suppressed and consistent with the prior ctypes usage.tests/e2e/_run_test_server.py (2)
27-29: Consider adding argument validation.The script will crash with an unhelpful
IndexErrorif the required host/port arguments are missing. A brief check would improve the developer experience when running the script incorrectly.🛡️ Optional: add argument validation
def main() -> None: + if len(sys.argv) < 3: + print("Usage: python _run_test_server.py <host> <port>", file=sys.stderr) + sys.exit(1) host = sys.argv[1] port = int(sys.argv[2])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/_run_test_server.py` around lines 27 - 29, The main() function reads host/port from sys.argv without validation which raises IndexError if arguments are missing; update main (or the top-level argument handling) to validate sys.argv length (or use argparse) before accessing sys.argv[1] and sys.argv[2], print a clear usage message including expected host and port, and exit with a non-zero status when arguments are missing or port is not an integer; reference the main function and the host/port variables derived from sys.argv to locate where to add the checks and error handling.
22-24: Rename component to PascalCase.Per project conventions, functions decorated with
@componentshould use PascalCase since they behave like components/classes.♻️ Proposed fix
`@component` -def test_app() -> None: +def TestApp() -> None: Label(text="Hello from SSR")And update the reference at line 43:
- app_instance = App(test_app) + app_instance = App(TestApp)And line 50:
- test_app, + TestApp,Based on learnings: "In Trellis, any function decorated with component should be named using PascalCase (e.g., RouterDemo, HelloWorld, WidgetShowcase) since these behave like components/classes rather than plain functions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/_run_test_server.py` around lines 22 - 24, Rename the `@component` function test_app to PascalCase (e.g., TestApp) and update all references/usages to the new name; specifically change the function definition def test_app(...) -> None: to def TestApp(...) -> None: (keep the `@component` decorator) and replace every call/import/usage of "test_app" (including the references mentioned in the diff) with "TestApp" so the component follows project naming conventions.src/trellis/platforms/common/ssr_utils.py (1)
17-24: Consider reusing_get_versionfromhandler.py.This function duplicates the implementation in
src/trellis/platforms/common/handler.py(lines 63-68). Consider extracting to a shared location to avoid drift.♻️ Suggested refactor
-def _get_version() -> str: - """Get package version from metadata.""" - try: - from importlib.metadata import version as get_package_version # noqa: PLC0415 - - return get_package_version("trellis") - except Exception: - return "0.0.0" +from trellis.platforms.common.handler import _get_versionOr extract to a shared utility module if
_get_versionshould remain private.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/common/ssr_utils.py` around lines 17 - 24, The _get_version implementation in ssr_utils duplicates the same function in handler; remove the duplicate by centralizing the logic: create or use a single shared function (e.g., a new get_version utility in a common utils module) or import the existing _get_version from the handler module, then update ssr_utils._get_version to call or re-export that shared function; ensure the shared function uses importlib.metadata.version("trellis") with the same fallback and update any imports and tests accordingly.src/trellis/platforms/server/ssr_cache.py (1)
19-33: Declare the cache fields on the class.
_cache,_lock, and_max_entriesare owned instance attributes, but their declarations still live inside__init__. Move the annotations to the class body to match the repo rule for plain classes. As per coding guidelines, "Declare all owned instance attributes in the class body with type annotations."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/ssr_cache.py` around lines 19 - 33, Move the instance attribute annotations for SSRCache out of __init__ into the class body: declare _cache: dict[tuple[str, str], str], _lock: threading.Lock, and _max_entries: int as class-level annotations (without initializing them) and leave initialization of their values inside __init__ (self._cache = {}, self._lock = threading.Lock(), self._max_entries = max_entries); update the SSRCache class header to include these annotations so the class follows the repo rule of declaring owned instance attributes in the class body while keeping construction logic in __init__.src/trellis/platforms/server/session_store.py (1)
31-38: Declare the store fields on the class.
_ttl,_entries, and_lockare introduced inside__init__instead of the class body. As per coding guidelines, "Declare all owned instance attributes in the class body with type annotations."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/session_store.py` around lines 31 - 38, Declare the owned instance attributes on the SessionStore class body by adding typed declarations for _ttl: float, _entries: dict[str, SessionEntry], and _lock: threading.Lock (or Optional[threading.Lock] if you prefer) above __init__; keep the actual assignments inside __init__ (self._ttl = ttl_seconds, self._entries = {}, self._lock = threading.Lock()) so the class has explicit attribute annotations while initialization remains in __init__ — update the SessionStore class definition (symbols: SessionStore, _ttl, _entries, _lock, __init__) accordingly.src/trellis/platforms/server/platform.py (1)
160-163: Compute the static directory once.This path is recomputed again at Line 192 before mounting
/static. Reusing a singlestaticvalue keeps SSR bundle discovery and mounted assets in lockstep, and avoids depending oncreate_static_dir()being perfectly idempotent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/platform.py` around lines 160 - 163, Compute the static directory once and reuse it instead of calling create_static_dir() twice: move the assignment static = static_dir or create_static_dir() to a single common point used both for SSR bundle discovery (ssr_bundle = static / "ssr.js" when ssr is true) and later for mounting /static, and remove the second recomputation; update any code paths that reference the old second call so they use the shared static variable (referencing ssr, static_dir, create_static_dir(), and ssr_bundle to locate the changes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/trellis/bundler/steps.py`:
- Around line 852-854: The code sets ctx.template_context["ssr_enabled"] True
even when renderer.render(tree) returns empty markup; change the logic so you
capture the rendered HTML from renderer.render(tree) into a variable (e.g.,
rendered_html), set ctx.template_context["ssr_html"] to rendered_html or "", set
ctx.template_context["ssr_enabled"] to True only when rendered_html is non-empty
(e.g., bool(rendered_html)), and only populate ctx.template_context["ssr_data"]
(via build_dehydration_data(None, wire_patches)) when ssr_enabled is True to
avoid enabling hydration with no server HTML; look for renderer.render,
build_dehydration_data, and the ssr_* keys to implement this change.
In `@src/trellis/platforms/browser/serve_platform.py`:
- Around line 124-127: The app-mode step list always includes SSRBundleBuildStep
and SSRPreRenderStep which ignores config.ssr; modify the code that composes the
steps (the list containing SSRBundleBuildStep(), StaticFileCopyStep(),
IconAssetStep(icon_path=config.icon), SSRPreRenderStep()) to conditionally
append or include SSRBundleBuildStep and SSRPreRenderStep only when config.ssr
is truthy — e.g., check config.ssr before adding these two steps so setting
ssr=False fully disables SSR for serve_platform.py.
In `@src/trellis/platforms/server/platform.py`:
- Around line 114-115: Validate session_ttl at the start of the run() method and
reject non-positive values to prevent a busy-loop in _session_cleanup_loop():
ensure session_ttl > 0 before using it for SessionStore TTL and asyncio.sleep;
if the value is ≤ 0, raise a ValueError (or otherwise fail-fast) with a clear
message referencing session_ttl so callers cannot start the server with an
invalid TTL.
In `@src/trellis/platforms/server/session_store.py`:
- Around line 44-62: Expired sessions are currently dropped without handing back
the underlying SessionEntry (and its RenderSession) so background tasks never
get shutdown; modify pop and cleanup_expired to return expired SessionEntry
objects instead of discarding them: in pop(session_id) when an entry exists but
now - entry.created_at > self._ttl, remove it from self._entries and return the
expired SessionEntry (so the caller can call its async shutdown on the
RenderSession), and change cleanup_expired() to collect expired SessionEntry
instances into a list, delete them from self._entries while holding the lock,
and return that list of SessionEntry objects for the caller to await shutdown on
each (update the function signatures and callers accordingly; reference symbols:
pop, cleanup_expired, SessionEntry and RenderSession.shutdown).
In `@src/trellis/platforms/server/ssr_cache.py`:
- Around line 30-33: The constructor __init__ must validate the max_entries
parameter to avoid negative/zero sizes causing eviction from an empty cache; add
a check in __init__ that raises a ValueError if max_entries <= 0 (or
alternatively accept 0 and explicitly document/implement it as "disabled"
behavior), assign the validated value to self._max_entries, and ensure the put()
logic that uses self._max_entries behaves consistently with this decision (i.e.,
if you choose to disallow <=0 then put() can assume _max_entries > 0).
In `@src/trellis/platforms/server/ssr_renderer.py`:
- Around line 75-77: In _start_locked() replace the call to self.stop() (while
holding self._lock) with a call to _stop_locked() to avoid deadlocking on the
non-reentrant threading.Lock; locate the failure branch where _wait_for_health()
returns False and invoke _stop_locked() instead of self.stop(), mirroring the
pattern used in _try_restart() which already uses _stop_locked(), so the locked
shutdown logic runs without re-acquiring self._lock.
In `@src/trellis/utils/subprocess.py`:
- Line 24: The Popen[bytes] return annotation is incorrect because callers may
pass text/encoding kwargs; update the annotations to reflect either a type
variable or a union of str|bytes: define a type variable (e.g. P = TypeVar("P",
str, bytes)) and use subprocess.Popen[P] for the return type of
start_child_process, stop_child_process, and the internal _stop_posix /
_stop_windows functions, or alternatively annotate them as
subprocess.Popen[Union[str, bytes]] to allow both modes; ensure all four
function signatures are changed consistently.
In `@tests/e2e/test_ssr_hydration.py`:
- Around line 60-64: The test currently skips when the test server fails to
start (using pytest.skip), which hides regressions; modify the block that calls
_wait_for_server(url, _STARTUP_TIMEOUT) so that after proc.kill() and capturing
stdout via proc.communicate(timeout=5) you call pytest.fail with the decoded
output (instead of pytest.skip) to ensure the test fails and surfaces the server
startup logs; update references in this block (_wait_for_server,
_STARTUP_TIMEOUT, proc.kill(), proc.communicate, pytest.skip) accordingly.
---
Outside diff comments:
In `@src/trellis/platforms/common/handler.py`:
- Around line 387-400: The initial-render branch skips router setup but the
normal fresh-session branch returns immediately after computing wire_patches,
leaving CSR sessions without navigation callbacks; after calling
render(self.session) and before returning PatchMessage(patches=wire_patches)
invoke self._setup_router_callbacks() so that
_on_navigate/_on_go_back/_on_go_forward are installed for non-SSR sessions
(ensure this happens after _serialize_patches(render_patches, self.session) and
before returning the PatchMessage).
---
Duplicate comments:
In `@src/trellis/platforms/server/routes.py`:
- Around line 27-35: The SPA fallback gate in _is_document_request should be
tightened: update the _is_document_request(request: Request) function (and the
equivalent logic at the other occurrence) to only return True for navigational
requests by requiring request.method to be GET or HEAD, ensuring "text/html" is
in request.headers.get("accept", ""), and excluding obvious API or asset
requests (e.g. request.url.path startswith "/api" or the path has a file
extension like ".js", ".css", ".png" — detect by checking for a '.' after the
last '/' in the path). Implement these checks together so only true browser
navigations trigger the SPA/SSR fallback.
- Around line 69-87: The synchronous SSR path in _ssr_or_static_response (used
by index() and spa_fallback_handler) blocks the ASGI event loop because
ssr.render_to_response (which leads to render_for_ssr and a blocking
httpx.Client.post to the Bun sidecar) runs on the main thread; change this by
offloading the blocking work to a thread pool or converting the
orchestrator/renderer to async: either call the blocking ssr.render_to_response
via loop.run_in_executor (or asyncio.to_thread) and await it, or
implement/replace ssr.render_to_response with an async render_to_response_async
that uses an async HTTP client, and update
_get_ssr_orchestrator/_ssr_or_static_response to await the async path; ensure
headers and exception handling remain the same.
In `@src/trellis/platforms/server/ssr.py`:
- Around line 73-91: The code currently stores sessions and injects the
dehydration script even when _get_ssr_html(...) returns an empty string (no SSR
available), causing the client to attempt hydration with no SSR markup; change
render logic so you first call ssr_html = self._get_ssr_html(...), then only
create and store SessionEntry via self._session_store.store(session_id, entry),
build_dehydration_data(...), and inject the dehydration_script when ssr_html is
non-empty; if ssr_html is empty, skip storing the session and skip adding
window.__TRELLIS_SSR__ (leave the template without the SSR outlet/dehydration
injection) — apply the same guard to the second identical block around
build_dehydration_data/_session_store usage.
In `@tests/e2e/test_ssr_hydration.py`:
- Around line 85-89: The current skip logic in the result.returncode == 2 branch
is too broad because it treats any "Cannot find module" as Playwright missing;
update the condition in tests/e2e/test_ssr_hydration.py (the block handling
result.returncode == 2 and the stderr variable) to only skip when stderr
explicitly indicates Playwright is missing — e.g., stderr contains "Playwright
not installed" or matches "Cannot find module.*playwright" (use a regex/search
rather than a plain "Cannot find module" substring check); leave other errors to
hit pytest.fail so real harness regressions are not masked.
---
Nitpick comments:
In `@src/trellis/platforms/common/ssr_utils.py`:
- Around line 17-24: The _get_version implementation in ssr_utils duplicates the
same function in handler; remove the duplicate by centralizing the logic: create
or use a single shared function (e.g., a new get_version utility in a common
utils module) or import the existing _get_version from the handler module, then
update ssr_utils._get_version to call or re-export that shared function; ensure
the shared function uses importlib.metadata.version("trellis") with the same
fallback and update any imports and tests accordingly.
In `@src/trellis/platforms/server/platform.py`:
- Around line 160-163: Compute the static directory once and reuse it instead of
calling create_static_dir() twice: move the assignment static = static_dir or
create_static_dir() to a single common point used both for SSR bundle discovery
(ssr_bundle = static / "ssr.js" when ssr is true) and later for mounting
/static, and remove the second recomputation; update any code paths that
reference the old second call so they use the shared static variable
(referencing ssr, static_dir, create_static_dir(), and ssr_bundle to locate the
changes).
In `@src/trellis/platforms/server/session_store.py`:
- Around line 31-38: Declare the owned instance attributes on the SessionStore
class body by adding typed declarations for _ttl: float, _entries: dict[str,
SessionEntry], and _lock: threading.Lock (or Optional[threading.Lock] if you
prefer) above __init__; keep the actual assignments inside __init__ (self._ttl =
ttl_seconds, self._entries = {}, self._lock = threading.Lock()) so the class has
explicit attribute annotations while initialization remains in __init__ — update
the SessionStore class definition (symbols: SessionStore, _ttl, _entries, _lock,
__init__) accordingly.
In `@src/trellis/platforms/server/ssr_cache.py`:
- Around line 19-33: Move the instance attribute annotations for SSRCache out of
__init__ into the class body: declare _cache: dict[tuple[str, str], str], _lock:
threading.Lock, and _max_entries: int as class-level annotations (without
initializing them) and leave initialization of their values inside __init__
(self._cache = {}, self._lock = threading.Lock(), self._max_entries =
max_entries); update the SSRCache class header to include these annotations so
the class follows the repo rule of declaring owned instance attributes in the
class body while keeping construction logic in __init__.
In `@src/trellis/utils/subprocess.py`:
- Around line 241-242: The assignment to kernel32.CloseHandle.restype triggers
the same type-checker warning as earlier; update the CloseHandle.restype line in
subprocess.py (the kernel32 = ctypes.WinDLL(...) / kernel32.CloseHandle.restype
= ...) to include the same ignore comment used on line 138 (append # type:
ignore[attr-defined]) so the static analyzer is suppressed and consistent with
the prior ctypes usage.
In `@tests/e2e/_run_test_server.py`:
- Around line 27-29: The main() function reads host/port from sys.argv without
validation which raises IndexError if arguments are missing; update main (or the
top-level argument handling) to validate sys.argv length (or use argparse)
before accessing sys.argv[1] and sys.argv[2], print a clear usage message
including expected host and port, and exit with a non-zero status when arguments
are missing or port is not an integer; reference the main function and the
host/port variables derived from sys.argv to locate where to add the checks and
error handling.
- Around line 22-24: Rename the `@component` function test_app to PascalCase
(e.g., TestApp) and update all references/usages to the new name; specifically
change the function definition def test_app(...) -> None: to def TestApp(...) ->
None: (keep the `@component` decorator) and replace every call/import/usage of
"test_app" (including the references mentioned in the diff) with "TestApp" so
the component follows project naming conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 293d8f29-024d-4854-b559-1ee464ae4fd5
📒 Files selected for processing (37)
pyproject.tomlsrc/trellis/app/config.pysrc/trellis/bundler/__init__.pysrc/trellis/bundler/steps.pysrc/trellis/cli/run.pysrc/trellis/core/rendering/__init__.pysrc/trellis/core/rendering/ssr.pysrc/trellis/platforms/browser/client/src/TrellisApp.tsxsrc/trellis/platforms/browser/client/src/index.html.j2src/trellis/platforms/browser/client/src/main.tsxsrc/trellis/platforms/browser/serve_platform.pysrc/trellis/platforms/common/client/src/TrellisRoot.tsxsrc/trellis/platforms/common/client/src/core/store.tssrc/trellis/platforms/common/client/src/types.tssrc/trellis/platforms/common/handler.pysrc/trellis/platforms/common/messages.pysrc/trellis/platforms/common/ssr_utils.pysrc/trellis/platforms/desktop/client/src/index.html.j2src/trellis/platforms/desktop/client/src/main.tsxsrc/trellis/platforms/desktop/platform.pysrc/trellis/platforms/server/client/src/TrellisClient.tssrc/trellis/platforms/server/client/src/index.html.j2src/trellis/platforms/server/client/src/main.tsxsrc/trellis/platforms/server/client/src/ssr-entry.tsxsrc/trellis/platforms/server/handler.pysrc/trellis/platforms/server/platform.pysrc/trellis/platforms/server/routes.pysrc/trellis/platforms/server/session_store.pysrc/trellis/platforms/server/ssr.pysrc/trellis/platforms/server/ssr_cache.pysrc/trellis/platforms/server/ssr_renderer.pysrc/trellis/utils/hot_reload.pysrc/trellis/utils/subprocess.pytests/e2e/_run_test_server.pytests/e2e/check_hydration.jstests/e2e/test_ssr_hydration.pytests/py/unit/test_build_config.py
🚧 Files skipped from review as they are similar to previous changes (6)
- src/trellis/platforms/server/client/src/ssr-entry.tsx
- src/trellis/utils/hot_reload.py
- tests/e2e/check_hydration.js
- src/trellis/platforms/common/messages.py
- src/trellis/cli/run.py
- tests/py/unit/test_build_config.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/trellis/platforms/common/handler.py`:
- Around line 373-376: The code claims to set up router callbacks in both SSR
and full-render paths but currently only calls _setup_router_callbacks() in the
SSR-skip branch; update the full initial-render path (the branch where
root_element_id is not set and the code performs the full initial render) to
invoke self._setup_router_callbacks() after the render is completed (same place
where the SSR path calls it) so router history is wired for non-resumed
sessions; ensure the call is added in the method that handles initial rendering
and mirrored for the alternative branch referenced near the logic using
root_element_id so both paths call _setup_router_callbacks().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2bc0751-b89f-46fb-b79f-2c089f615ca2
📒 Files selected for processing (2)
src/trellis/platforms/common/handler.pysrc/trellis/utils/subprocess.py
- SSRBundleBuildStep: clarify docstring (not shipped to client) - SSRPreRenderStep: move renderer.start() inside try; only set ssr_enabled when HTML is non-empty - MessageHandler: extract _try_resume_session/_create_session helpers; add _ssr_resumed flag to skip initial_render for SSR sessions; set up router callbacks in non-SSR initial render path - ssr_utils: update docstring (used by all platforms); document escaping approach - Browser serve_platform: gate SSR steps on config.ssr - Server platform: move imports to top level; extract _setup_ssr() - ssr-entry.tsx: document that it's not shipped to client
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/trellis/platforms/server/platform.py (1)
159-160:⚠️ Potential issue | 🟡 MinorValidate
session_ttlto prevent busy-loop in cleanup task.A non-positive
session_ttlwould cause_session_cleanup_loopto spin tightly sinceasyncio.sleep(0)returns immediately. Add validation at the start ofrun().🛡️ Suggested fix
) -> None: """Start FastAPI server with WebSocket support. ... session_ttl: Session time-to-live in seconds (default 300) """ + if session_ttl <= 0: + raise ValueError("session_ttl must be greater than 0") + # Start hot reload if enabled if hot_reload:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/platform.py` around lines 159 - 160, The run() entry should validate session_ttl to prevent a busy-loop in _session_cleanup_loop when session_ttl is non-positive; add a guard at the start of run() that checks the session_ttl parameter (or self.session_ttl) and either raise a ValueError for non-positive values or coerce it to a safe minimum (e.g., max(1, session_ttl)) before spawning the cleanup task so asyncio.sleep is never called with 0 or negative.
🧹 Nitpick comments (1)
src/trellis/platforms/common/ssr_utils.py (1)
17-24: Consider deduplicating_get_version()helper.This function is nearly identical to
_get_version()insrc/trellis/platforms/common/handler.py(lines 63-68). Consider extracting to a shared utility to avoid duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/common/ssr_utils.py` around lines 17 - 24, Duplicate helper _get_version should be extracted to a single shared utility and both callers should import it: create a shared function (e.g., get_package_version or keep _get_version) in a common module (e.g., a new common version util) that contains the try/importlib.metadata logic and fallback "0.0.0", then replace the local implementations in ssr_utils._get_version and the one in handler (the other _get_version) to import and call the shared function instead of duplicating the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/trellis/platforms/server/platform.py`:
- Around line 159-160: The run() entry should validate session_ttl to prevent a
busy-loop in _session_cleanup_loop when session_ttl is non-positive; add a guard
at the start of run() that checks the session_ttl parameter (or
self.session_ttl) and either raise a ValueError for non-positive values or
coerce it to a safe minimum (e.g., max(1, session_ttl)) before spawning the
cleanup task so asyncio.sleep is never called with 0 or negative.
---
Nitpick comments:
In `@src/trellis/platforms/common/ssr_utils.py`:
- Around line 17-24: Duplicate helper _get_version should be extracted to a
single shared utility and both callers should import it: create a shared
function (e.g., get_package_version or keep _get_version) in a common module
(e.g., a new common version util) that contains the try/importlib.metadata logic
and fallback "0.0.0", then replace the local implementations in
ssr_utils._get_version and the one in handler (the other _get_version) to import
and call the shared function instead of duplicating the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f0595a8-0e5c-4971-b25e-fa4a0cc652d4
📒 Files selected for processing (7)
in-progress-review.mdsrc/trellis/bundler/steps.pysrc/trellis/platforms/browser/serve_platform.pysrc/trellis/platforms/common/handler.pysrc/trellis/platforms/common/ssr_utils.pysrc/trellis/platforms/server/client/src/ssr-entry.tsxsrc/trellis/platforms/server/platform.py
All SSR HTML uses CSS variables, so light and dark themes produce identical markup. Remove theme detection from routes, theme params from the orchestrator, and simplify the cache to key by route only. This halves the cache entries per route.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/trellis/platforms/server/ssr_cache.py (1)
33-36:⚠️ Potential issue | 🟡 MinorReject non-positive cache sizes.
max_entries <= 0makes the firstput()evict from an empty dict and raiseStopIteration. Guard this in__init__, or explicitly define0as “cache disabled.”Suggested fix
def __init__(self, max_entries: int = _DEFAULT_MAX_ENTRIES) -> None: + if max_entries <= 0: + raise ValueError("max_entries must be greater than 0") self._cache: dict[str, str] = {} self._lock = threading.Lock() self._max_entries = max_entries🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/ssr_cache.py` around lines 33 - 36, The constructor currently allows non-positive max_entries which causes put() to evict from an empty dict; in __init__ of the SSR cache class validate max_entries and reject <= 0 (raise ValueError) or explicitly treat 0 as "disabled" by storing _max_entries = 0 and updating put() to skip eviction when _max_entries == 0; locate the __init__ method and the put() method (references: __init__, self._max_entries, put()) and implement the chosen guard so no StopIteration can occur.src/trellis/platforms/server/routes.py (1)
27-34:⚠️ Potential issue | 🟠 MajorOnly hijack true document navigations here.
_is_document_request()only checksAccept, and the non-document branch synthesizesHTMLResponse("Not Found"). That lets HTML-acceptingPOST/HEADmisses turn into SPA responses, while API/static 404s lose FastAPI's normal payload and content type. Restrict this toGET/HEADdocument requests and delegate every other miss to FastAPI's default HTTP exception handler.Minimal fix
-from fastapi.responses import HTMLResponse +from fastapi.exception_handlers import http_exception_handler +from fastapi.responses import HTMLResponse, Response @@ def _is_document_request(request: Request) -> bool: @@ accept = request.headers.get("accept", "") - return "text/html" in accept + return request.method in {"GET", "HEAD"} and "text/html" in accept @@ - async def spa_fallback_handler(request: Request, exc: StarletteHTTPException) -> HTMLResponse: + async def spa_fallback_handler(request: Request, exc: StarletteHTTPException) -> Response: """Return SPA HTML for document 404s to support client-side routing.""" if not _is_document_request(request): - return HTMLResponse(content="Not Found", status_code=404) + return await http_exception_handler(request, exc) return _ssr_or_static_response(request)Also applies to: 49-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/routes.py` around lines 27 - 34, The current _is_document_request(request: Request) only checks the Accept header and should instead only return True for actual browser navigations (GET or HEAD that accept text/html); update _is_document_request to verify request.method is "GET" or "HEAD" in addition to "text/html" in request.headers.get("accept", ""), and leave all other methods (e.g., POST, PUT, etc.) to fall through so the route's non-document branch delegates to FastAPI's default HTTP exception handling (do not synthesize an HTMLResponse("Not Found") for non-GET/HEAD requests).
🧹 Nitpick comments (2)
in-progress-review.md (2)
76-83: Consider reorganizing the "Already resolved" section.The items 24-28 create a numbering gap between item 23 and item 29, which could cause confusion when referencing items. Consider either:
- Moving these items into the "Actionable" section with strikethrough formatting if they were originally part of that list, or
- Removing this section entirely if these items are truly redundant (since items marked "✅ Previously fixed" already appear in the Actionable section)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@in-progress-review.md` around lines 76 - 83, The "Already resolved" subsection creates a numbering gap (items 24–28) and should be reorganized: either move items 24–28 into the "Actionable" list with strikethrough formatting so numbering remains contiguous with items before/after, or delete the entire "Already resolved" subsection if those entries are redundant with the "✅ **Previously fixed**" entries; update the "Already resolved" header and the lists surrounding it (the section containing items 24-28 and the "Actionable" list) so numbering flows without gaps and references remain unambiguous.
64-74: Add priority/timeline context to open items.Three items remain open without clear indication of whether they block this PR or can be deferred to follow-up work:
- Item 20: Platform JS audit for shared SSR/hydration code
- Items 22-23: Async subprocess/renderer operations (already linked)
Consider adding one of these markers to each open item:
- Blocking — must be resolved before merge
- Follow-up — tracked in a separate issue for post-merge work
This will help clarify what remains for PR approval vs. future improvements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@in-progress-review.md` around lines 64 - 74, Update the open checklist entries to include clear priority/timeline markers so reviewers know what blocks merging versus what can be deferred: for the entry referencing main.tsx:48 (emma) add either "→ **Open — Blocking**" or "→ **Open — Follow-up**" after the Platform JS audit line; for subprocess.py:110 (emma) and ssr_renderer.py:28 (emma) append the same marker (or different ones if one is blocking and the other is follow-up) next to their "→ **Open**" statuses and, if choosing Follow-up, add a short note referencing the linked work (e.g., "tracked in issue #...") so items 20, 22, and 23 visibly indicate whether they must be resolved pre-merge or can be addressed post-merge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/trellis/platforms/server/routes.py`:
- Around line 49-53: spa_fallback_handler and index call the blocking helper
_ssr_or_static_response directly from async handlers; change both to offload
that call to a thread pool using starlette.concurrency.run_in_threadpool (i.e.,
await run_in_threadpool(_ssr_or_static_response, request)) so disk I/O
(get_index_html/Path.read_text) and the Bun IPC/httpx POST work off the event
loop; update imports if necessary and ensure both async handlers await the
run_in_threadpool call rather than calling _ssr_or_static_response
synchronously.
In `@src/trellis/platforms/server/ssr.py`:
- Around line 68-86: The code currently stores SessionEntry via
self._session_store.store(session_id, entry) before obtaining SSR HTML and
dehydration payload; change render_to_response so that you call
self._session_store.store(...) only after ssr_html = self._get_ssr_html(path,
session) returns a non-empty/non-None value and after dehydration_data =
build_dehydration_data(session_id, wire_patches) is successfully created; if
_get_ssr_html returns None/"" then return None or raise so the plain-CSR path
can be taken and no SessionEntry is stored; apply the same ordering and guard to
the alternate branch around the second occurrence (lines analogous to 92-108) so
storage happens only after SSR HTML and dehydration payload are validated.
---
Duplicate comments:
In `@src/trellis/platforms/server/routes.py`:
- Around line 27-34: The current _is_document_request(request: Request) only
checks the Accept header and should instead only return True for actual browser
navigations (GET or HEAD that accept text/html); update _is_document_request to
verify request.method is "GET" or "HEAD" in addition to "text/html" in
request.headers.get("accept", ""), and leave all other methods (e.g., POST, PUT,
etc.) to fall through so the route's non-document branch delegates to FastAPI's
default HTTP exception handling (do not synthesize an HTMLResponse("Not Found")
for non-GET/HEAD requests).
In `@src/trellis/platforms/server/ssr_cache.py`:
- Around line 33-36: The constructor currently allows non-positive max_entries
which causes put() to evict from an empty dict; in __init__ of the SSR cache
class validate max_entries and reject <= 0 (raise ValueError) or explicitly
treat 0 as "disabled" by storing _max_entries = 0 and updating put() to skip
eviction when _max_entries == 0; locate the __init__ method and the put() method
(references: __init__, self._max_entries, put()) and implement the chosen guard
so no StopIteration can occur.
---
Nitpick comments:
In `@in-progress-review.md`:
- Around line 76-83: The "Already resolved" subsection creates a numbering gap
(items 24–28) and should be reorganized: either move items 24–28 into the
"Actionable" list with strikethrough formatting so numbering remains contiguous
with items before/after, or delete the entire "Already resolved" subsection if
those entries are redundant with the "✅ **Previously fixed**" entries; update
the "Already resolved" header and the lists surrounding it (the section
containing items 24-28 and the "Actionable" list) so numbering flows without
gaps and references remain unambiguous.
- Around line 64-74: Update the open checklist entries to include clear
priority/timeline markers so reviewers know what blocks merging versus what can
be deferred: for the entry referencing main.tsx:48 (emma) add either "→ **Open —
Blocking**" or "→ **Open — Follow-up**" after the Platform JS audit line; for
subprocess.py:110 (emma) and ssr_renderer.py:28 (emma) append the same marker
(or different ones if one is blocking and the other is follow-up) next to their
"→ **Open**" statuses and, if choosing Follow-up, add a short note referencing
the linked work (e.g., "tracked in issue #...") so items 20, 22, and 23 visibly
indicate whether they must be resolved pre-merge or can be addressed post-merge.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f2a7fc6c-f0ff-4984-9f30-b1f4819c1c24
📒 Files selected for processing (6)
in-progress-review.mdsrc/trellis/platforms/server/routes.pysrc/trellis/platforms/server/ssr.pysrc/trellis/platforms/server/ssr_cache.pytests/py/unit/test_ssr_cache.pytests/py/unit/test_ssr_orchestrator.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/py/unit/test_ssr_orchestrator.py
- BuildStep.run() is now async; all 15 build steps updated - build() is now async, awaits each step - AppLoader.bundle() is now async - CLI commands (run, bundle, package) use asyncio.run() - SSRRenderer uses asyncio subprocess + httpx.AsyncClient instead of blocking Popen + sync httpx.Client - SSROrchestrator.render_to_response() is now async - Added start_child_process_async/stop_child_process_async to subprocess utils using asyncio.create_subprocess_exec - All test call sites updated to asyncio.run() or AsyncMock
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/trellis/platforms/server/ssr_renderer.py (1)
81-83:⚠️ Potential issue | 🔴 CriticalDeadlock:
self.stop()called while holdingself._lock.
_start_locked()is called withself._lockheld (fromstart()at line 54-55). When the health check fails at line 81, callingawait self.stop()attempts to acquireself._lockagain at line 145. Sinceasyncio.Lockis not reentrant, this causes a deadlock.Use
_stop_locked()instead, which is the correct pattern already used in_try_restart()at line 137.🔒 Proposed fix
if not await self._wait_for_health(): - await self.stop() + await self._stop_locked() raise RuntimeError("SSR renderer failed health check")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/ssr_renderer.py` around lines 81 - 83, _in _start_locked(), you're calling await self.stop() while still holding self._lock which deadlocks because asyncio.Lock isn't reentrant; replace that call with await self._stop_locked() (the helper that assumes the lock is already held) to mirror the pattern used in _try_restart() and avoid attempting to re-acquire self._lock.src/trellis/platforms/server/platform.py (1)
159-160:⚠️ Potential issue | 🟠 MajorValidate
session_ttlto prevent busy-loop in cleanup task.The
session_ttlparameter is used as the sleep interval in_session_cleanup_loop(). A non-positive value would cause the loop to spin continuously sinceasyncio.sleep(0)orasyncio.sleep(negative)returns immediately.🛡️ Proposed fix to add validation
async def run( self, root_component: Callable[[], Element], app_wrapper: AppWrapper, *, host: str = "127.0.0.1", port: int | None = None, static_dir: Path | None = None, batch_delay: float = 1.0 / 30, hot_reload: bool = True, ssr: bool = True, session_ttl: int = 300, **_kwargs: Any, # Ignore other platform args ) -> None: """Start FastAPI server with WebSocket support. ... """ + if session_ttl <= 0: + raise ValueError("session_ttl must be greater than 0") + # Start hot reload if enabled if hot_reload:Also applies to: 241-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/platform.py` around lines 159 - 160, session_ttl is used as the sleep interval in _session_cleanup_loop and must be validated to avoid a busy-loop; add a check where session_ttl is accepted (e.g., the class constructor or factory that defines the parameter) to ensure session_ttl is a positive integer (>0) and raise ValueError (or TypeError for non-int) with a clear message if invalid, and update any other locations that accept session_ttl (the other occurrence around the secondary parameter handling) to perform the same validation so _session_cleanup_loop always receives a positive sleep interval.src/trellis/utils/subprocess.py (1)
170-175:⚠️ Potential issue | 🟠 MajorDefine
argtypesforkernel32calls to avoid unsafe argument marshaling.Lines [170]-[175] set
restypeonly. Withoutargtypes,ctypesmay coerce HANDLE arguments incorrectly on 64-bit Windows.💡 Proposed fix
kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) # type: ignore[attr-defined] kernel32.CreateJobObjectW.restype = ctypes.wintypes.HANDLE + kernel32.CreateJobObjectW.argtypes = [ctypes.c_void_p, ctypes.wintypes.LPCWSTR] kernel32.SetInformationJobObject.restype = ctypes.wintypes.BOOL + kernel32.SetInformationJobObject.argtypes = [ + ctypes.wintypes.HANDLE, + ctypes.wintypes.INT, + ctypes.c_void_p, + ctypes.wintypes.DWORD, + ] kernel32.OpenProcess.restype = ctypes.wintypes.HANDLE + kernel32.OpenProcess.argtypes = [ + ctypes.wintypes.DWORD, + ctypes.wintypes.BOOL, + ctypes.wintypes.DWORD, + ] kernel32.AssignProcessToJobObject.restype = ctypes.wintypes.BOOL + kernel32.AssignProcessToJobObject.argtypes = [ + ctypes.wintypes.HANDLE, + ctypes.wintypes.HANDLE, + ] kernel32.CloseHandle.restype = ctypes.wintypes.BOOL + kernel32.CloseHandle.argtypes = [ctypes.wintypes.HANDLE]#!/bin/bash # Verify current ctypes prototype coverage for Win32 APIs in this file. nl -ba src/trellis/utils/subprocess.py | sed -n '165,240p' echo "---- argtypes declarations ----" rg -n 'kernel32\..*\.argtypes\s*=' src/trellis/utils/subprocess.py echo "---- restype declarations ----" rg -n 'kernel32\..*\.restype\s*=' src/trellis/utils/subprocess.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/utils/subprocess.py` around lines 170 - 175, The kernel32 functions CreateJobObjectW, SetInformationJobObject, OpenProcess, AssignProcessToJobObject and CloseHandle currently only set restype; add explicit .argtypes declarations matching the Win32 signatures to avoid unsafe marshaling on 64-bit Windows (e.g., CreateJobObjectW(lpJobAttributes, lpName) -> (ctypes.c_void_p, ctypes.wintypes.LPCWSTR), SetInformationJobObject(hJob, infoClass, info, infoSize) -> (ctypes.wintypes.HANDLE, ctypes.c_int, ctypes.c_void_p, ctypes.wintypes.DWORD), OpenProcess(desiredAccess, inheritHandle, pid) -> (ctypes.wintypes.DWORD, ctypes.wintypes.BOOL, ctypes.wintypes.DWORD), AssignProcessToJobObject(hJob, hProcess) -> (ctypes.wintypes.HANDLE, ctypes.wintypes.HANDLE), CloseHandle(hObject) -> (ctypes.wintypes.HANDLE)); add these .argtypes next to the existing .restype lines for the functions named above.
🧹 Nitpick comments (4)
tests/e2e/_run_test_server.py (2)
22-24: Rename component function to PascalCase.Line 23 defines a
@componentfunction astest_app; Trellis component functions are expected to use PascalCase for consistency.♻️ Proposed rename
`@component` -def test_app() -> None: +def TestApp() -> None: w.Label(text="Hello from SSR")- app_instance = App(test_app) + app_instance = App(TestApp)- test_app, + TestApp,Based on learnings: “In Trellis, any function decorated with component should be named using PascalCase … enforce this convention across Python files in the repo.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/_run_test_server.py` around lines 22 - 24, The `@component` function currently named test_app should be renamed to PascalCase (e.g., TestApp) to follow Trellis conventions: change the function name from test_app to TestApp, update any imports or references that call or register test_app (search for "test_app") to the new name, and keep the `@component` decorator and body unchanged so behavior remains identical; ensure unit/e2e test harness or server startup code that expects the component is updated to reference TestApp.
14-20: Use canonical Trellis imports for component/widgets.Line 17 and Line 19 use non-canonical import paths for Trellis component/widget APIs. Please align with repository import conventions.
♻️ Proposed import/style adjustment
-from trellis.app.app import App from trellis.app.apploader import AppLoader, set_apploader from trellis.app.config import Config -from trellis.core.components.composition import component from trellis.platforms.server.platform import ServerPlatform -from trellis.widgets import Label +from trellis import App, component +from trellis import widgets as wdef test_app() -> None: - Label(text="Hello from SSR") + w.Label(text="Hello from SSR")As per coding guidelines: “Use canonical import style:
from trellis import App, component, Stateful… import widgets viafrom trellis import widgets as w”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/_run_test_server.py` around lines 14 - 20, The imports use non‑canonical paths; change them to the repository's canonical style by importing core APIs from the trellis top-level and widgets via an alias (e.g., replace explicit module imports of App, AppLoader, set_apploader, Config, component, ServerPlatform with top‑level imports like "from trellis import App, AppLoader, set_apploader, Config, component, ServerPlatform" and import widgets as an alias like "from trellis import widgets as w"), then update usages such as the Label symbol to use the widget alias (e.g., w.Label) so component/widget references match the project's canonical import conventions.src/trellis/platforms/server/routes.py (1)
68-68: Minor:get_index_html()performs synchronous file I/O in async handlers.
Path.read_text()insideget_index_html()is a blocking call. While unlikely to cause issues for a small file likeindex.html, it could briefly block the event loop under high concurrency.Consider caching the content in memory after the first read, or using
asyncio.to_thread()/run_in_threadpool()for the file read. This is a low-priority optimization.Also applies to: 74-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/routes.py` at line 68, get_index_html() currently uses Path.read_text() which does synchronous file I/O inside async request handlers (routes that pass html_template=get_index_html()), so change get_index_html to avoid blocking the event loop: either cache the file contents in a module-level variable after the first read (e.g., lazy_load_index_html) or perform the read off the loop with asyncio.to_thread()/run_in_threadpool and await the result; update callers that reference get_index_html() (the route handlers that set html_template=...) to call the async-safe variant or the cached value instead.src/trellis/platforms/server/ssr_renderer.py (1)
105-125: Consider the race window between lock release and HTTP request.The lock is released after capturing
clientat line 110, but the actual HTTP request happens outside the lock (lines 112-121). If_try_restart()runs concurrently (triggered by another request's failure), it could close and replaceself._clientwhile this request is in flight.This is a minor concern since the captured
clientreference remains valid for the duration of the request, and httpx handles connection closure gracefully. However, for robustness, consider keeping the lock held for the entire render operation or using a separate read-write lock pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/ssr_renderer.py` around lines 105 - 125, The render method currently releases self._lock before performing the HTTP call, allowing _try_restart to replace or close self._client mid-request; fix by keeping the lock held for the duration of the render operation (i.e., move the await client.post(...) and response handling inside the async with self._lock block) or alternatively replace self._lock with a read-write lock and acquire a read lock in render while _try_restart uses a write lock — reference symbols: render, self._lock, self._client, _try_restart.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/trellis/utils/subprocess.py`:
- Around line 268-278: The Windows job-handle cleanup leaves process._job_handle
set so subsequent cleanup can try to close the same/stale handle; after
successfully calling kernel32.CloseHandle(job_handle) (in the try block handling
process and _job_handle), clear or remove the attribute (e.g., set
process._job_handle = None or delattr(process, "_job_handle")) to make the
operation idempotent and avoid double-closing the handle; ensure this change
happens only after a successful CloseHandle call and keep the existing exception
fallback around kernel32.CloseHandle and imports.
In `@tests/e2e/_run_test_server.py`:
- Around line 27-30: The main function currently reads host and port directly
from sys.argv which can raise IndexError/ValueError; update main to validate
inputs before parsing by checking len(sys.argv) >= 3, parse host and port inside
a try/except that catches ValueError for int() conversion, and on error print a
clear usage/error message to stderr and call sys.exit(1). Locate the logic in
main (references to sys.argv, host, port and int(...)) and replace the direct
indexing/conversion with this guarded parsing so failures produce deterministic,
descriptive exits.
---
Duplicate comments:
In `@src/trellis/platforms/server/platform.py`:
- Around line 159-160: session_ttl is used as the sleep interval in
_session_cleanup_loop and must be validated to avoid a busy-loop; add a check
where session_ttl is accepted (e.g., the class constructor or factory that
defines the parameter) to ensure session_ttl is a positive integer (>0) and
raise ValueError (or TypeError for non-int) with a clear message if invalid, and
update any other locations that accept session_ttl (the other occurrence around
the secondary parameter handling) to perform the same validation so
_session_cleanup_loop always receives a positive sleep interval.
In `@src/trellis/platforms/server/ssr_renderer.py`:
- Around line 81-83: _in _start_locked(), you're calling await self.stop() while
still holding self._lock which deadlocks because asyncio.Lock isn't reentrant;
replace that call with await self._stop_locked() (the helper that assumes the
lock is already held) to mirror the pattern used in _try_restart() and avoid
attempting to re-acquire self._lock.
In `@src/trellis/utils/subprocess.py`:
- Around line 170-175: The kernel32 functions CreateJobObjectW,
SetInformationJobObject, OpenProcess, AssignProcessToJobObject and CloseHandle
currently only set restype; add explicit .argtypes declarations matching the
Win32 signatures to avoid unsafe marshaling on 64-bit Windows (e.g.,
CreateJobObjectW(lpJobAttributes, lpName) -> (ctypes.c_void_p,
ctypes.wintypes.LPCWSTR), SetInformationJobObject(hJob, infoClass, info,
infoSize) -> (ctypes.wintypes.HANDLE, ctypes.c_int, ctypes.c_void_p,
ctypes.wintypes.DWORD), OpenProcess(desiredAccess, inheritHandle, pid) ->
(ctypes.wintypes.DWORD, ctypes.wintypes.BOOL, ctypes.wintypes.DWORD),
AssignProcessToJobObject(hJob, hProcess) -> (ctypes.wintypes.HANDLE,
ctypes.wintypes.HANDLE), CloseHandle(hObject) -> (ctypes.wintypes.HANDLE)); add
these .argtypes next to the existing .restype lines for the functions named
above.
---
Nitpick comments:
In `@src/trellis/platforms/server/routes.py`:
- Line 68: get_index_html() currently uses Path.read_text() which does
synchronous file I/O inside async request handlers (routes that pass
html_template=get_index_html()), so change get_index_html to avoid blocking the
event loop: either cache the file contents in a module-level variable after the
first read (e.g., lazy_load_index_html) or perform the read off the loop with
asyncio.to_thread()/run_in_threadpool and await the result; update callers that
reference get_index_html() (the route handlers that set html_template=...) to
call the async-safe variant or the cached value instead.
In `@src/trellis/platforms/server/ssr_renderer.py`:
- Around line 105-125: The render method currently releases self._lock before
performing the HTTP call, allowing _try_restart to replace or close self._client
mid-request; fix by keeping the lock held for the duration of the render
operation (i.e., move the await client.post(...) and response handling inside
the async with self._lock block) or alternatively replace self._lock with a
read-write lock and acquire a read lock in render while _try_restart uses a
write lock — reference symbols: render, self._lock, self._client, _try_restart.
In `@tests/e2e/_run_test_server.py`:
- Around line 22-24: The `@component` function currently named test_app should be
renamed to PascalCase (e.g., TestApp) to follow Trellis conventions: change the
function name from test_app to TestApp, update any imports or references that
call or register test_app (search for "test_app") to the new name, and keep the
`@component` decorator and body unchanged so behavior remains identical; ensure
unit/e2e test harness or server startup code that expects the component is
updated to reference TestApp.
- Around line 14-20: The imports use non‑canonical paths; change them to the
repository's canonical style by importing core APIs from the trellis top-level
and widgets via an alias (e.g., replace explicit module imports of App,
AppLoader, set_apploader, Config, component, ServerPlatform with top‑level
imports like "from trellis import App, AppLoader, set_apploader, Config,
component, ServerPlatform" and import widgets as an alias like "from trellis
import widgets as w"), then update usages such as the Label symbol to use the
widget alias (e.g., w.Label) so component/widget references match the project's
canonical import conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 996e61a3-3511-4041-a40a-a86b8e4c899a
📒 Files selected for processing (22)
src/trellis/app/apploader.pysrc/trellis/bundler/build.pysrc/trellis/bundler/steps.pysrc/trellis/cli/bundle.pysrc/trellis/cli/package.pysrc/trellis/cli/run.pysrc/trellis/platforms/browser/build_steps.pysrc/trellis/platforms/server/platform.pysrc/trellis/platforms/server/routes.pysrc/trellis/platforms/server/ssr.pysrc/trellis/platforms/server/ssr_renderer.pysrc/trellis/utils/subprocess.pytests/e2e/_run_test_server.pytests/py/integration/test_platform_incremental_builds.pytests/py/unit/test_apploader.pytests/py/unit/test_browser_build_steps.pytests/py/unit/test_build_steps.pytests/py/unit/test_bundler_unit.pytests/py/unit/test_ssr_bundle_step.pytests/py/unit/test_ssr_orchestrator.pytests/py/unit/test_ssr_pre_render_step.pytests/py/unit/test_step_contracts.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/py/unit/test_ssr_bundle_step.py
- tests/py/unit/test_ssr_orchestrator.py
- init.ts: Symbol.dispose polyfill, registry init, theme CSS, console - ssr.ts: TrellisSSRData type, ssrData extraction, mountApp() helper that handles hydrate-vs-createRoot branching - ClientApp.tsx: shared App wrapper for server+desktop with client lifecycle management (connect/disconnect/error) All three platform entry points now import from common instead of duplicating initialization, SSR types, and hydration logic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/trellis/platforms/server/platform.py (1)
159-160:⚠️ Potential issue | 🟡 MinorAdd validation for
session_ttlto prevent busy-loop in cleanup task.A direct programmatic call like
ServerPlatform().run(..., session_ttl=0)would cause_session_cleanup_loopto spin tightly viaasyncio.sleep(0). While config-level validation may protect the CLI path, the public API should fail fast on invalid values.🛡️ Proposed fix
async def run( self, root_component: Callable[[], Element], app_wrapper: AppWrapper, *, host: str = "127.0.0.1", port: int | None = None, static_dir: Path | None = None, batch_delay: float = 1.0 / 30, hot_reload: bool = True, ssr: bool = True, session_ttl: int = 300, **_kwargs: Any, ) -> None: + if session_ttl <= 0: + raise ValueError("session_ttl must be greater than 0") + # Start hot reload if enabled🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/platform.py` around lines 159 - 160, Validate session_ttl early in the public API to avoid a busy-loop: in ServerPlatform.run (and/or ServerPlatform.__init__ if more appropriate) add a check that session_ttl is an integer > 0 and raise a ValueError with a clear message if not; this prevents _session_cleanup_loop from calling asyncio.sleep(0) and spinning tightly. Ensure the validation references the session_ttl parameter passed to run so callers receive a fast, explicit failure instead of triggering the cleanup loop.src/trellis/utils/subprocess.py (1)
76-93:⚠️ Potential issue | 🟠 MajorAsync stop path does not terminate process group on POSIX.
start_child_process_async()starts the process in a new session (line 72), butstop_child_process_async()only callsprocess.terminate()andprocess.kill(), which signal the child PID directly. Descendant processes (grandchildren) will survive, potentially leaving orphans.The sync
_stop_posix()correctly usesos.killpg()(lines 140-141, 147-148). The async variant should mirror this behavior.🔧 Proposed fix
async def stop_child_process_async( process: asyncio.subprocess.Process, timeout: float = 5.0 ) -> None: """Stop an async child process gracefully, escalating to SIGKILL on timeout.""" if process.returncode is not None: return try: - process.terminate() + if os.name == "nt": + process.terminate() + else: + import signal # noqa: PLC0415 + os.killpg(os.getpgid(process.pid), signal.SIGTERM) await asyncio.wait_for(process.wait(), timeout=timeout) except ProcessLookupError: pass except TimeoutError: try: - process.kill() + if os.name == "nt": + process.kill() + else: + import signal # noqa: PLC0415 + os.killpg(os.getpgid(process.pid), signal.SIGKILL) await process.wait() except ProcessLookupError: pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/utils/subprocess.py` around lines 76 - 93, The async shutdown in stop_child_process_async currently signals only the child PID; update it to mirror _stop_posix() by sending signals to the process group on POSIX: when process.pid is set and the platform supports process groups (e.g., os.name != "nt" or hasattr(os, "killpg")), call os.killpg(os.getpgid(process.pid), signal.SIGTERM) instead of process.terminate(), await process.wait() with asyncio.TimeoutError handling, and on timeout call os.killpg(..., signal.SIGKILL); fall back to process.terminate()/process.kill() for non-POSIX or if process.pid is missing, and keep the existing ProcessLookupError handling. Ensure imports for os and signal and use asyncio.TimeoutError in the except clause.
🧹 Nitpick comments (1)
src/trellis/platforms/common/client/src/ClientApp.tsx (1)
28-33: Connection error is logged but not surfaced to user.When
client.connect()rejects, the error is logged to console butsetErroris not called. The user sees no indication of the connection failure. Consider callingsetErrorwith a meaningful message:useEffect(() => { client.connect().catch((err) => { console.error("Failed to connect:", err); + setError(err instanceof Error ? err.message : "Connection failed"); }); return () => client.disconnect(); }, [client]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/common/client/src/ClientApp.tsx` around lines 28 - 33, The connection failure is only console.logged inside the useEffect that calls client.connect(); update that catch handler in the useEffect (the client.connect() promise) to call setError with a meaningful message (including err.message or err) and clear any connecting state (e.g., call setIsConnecting(false) if present) so the UI is updated; ensure the same is done for any thrown errors from client.connect() so the user sees the error and the component stops showing a loading/connecting state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/trellis/platforms/common/client/src/ClientApp.tsx`:
- Line 26: The client factory passed into ClientApp is unstable because
server/main.tsx supplies createClient as an inline arrow function, causing
ClientApp's useMemo (const client = useMemo(() => createClient(setError),
[createClient])) to recreate the client every render; fix by stabilizing the
factory in server/main.tsx—wrap the inline createClient arrow function in
useCallback so the reference passed into ClientApp remains stable (mirroring the
desktop entry's approach) and only changes when its true dependencies change.
In `@src/trellis/platforms/server/client/src/main.tsx`:
- Around line 10-22: The inline factory passed to ClientApp is recreated on
every render causing ServerTrellisClient to be rebuilt; wrap the createClient
factory in React's useCallback inside the App component to memoize it
(referencing the createClient prop passed to ClientApp and the
ServerTrellisClient constructor) and include ssrData?.sessionId and onError in
the useCallback dependency array so the factory only changes when those values
change.
In `@src/trellis/platforms/server/ssr_renderer.py`:
- Around line 81-83: The health-check failure path in _start_locked() calls
await self.stop() while the asyncio.Lock is held, causing a deadlock; replace
that call with a direct call to self._stop_locked() (as done in _try_restart())
so the locked shutdown runs without attempting to re-acquire the same
lock—update the failure branch in _start_locked() to call _stop_locked() instead
of await self.stop(), keeping the raise RuntimeError afterwards.
---
Duplicate comments:
In `@src/trellis/platforms/server/platform.py`:
- Around line 159-160: Validate session_ttl early in the public API to avoid a
busy-loop: in ServerPlatform.run (and/or ServerPlatform.__init__ if more
appropriate) add a check that session_ttl is an integer > 0 and raise a
ValueError with a clear message if not; this prevents _session_cleanup_loop from
calling asyncio.sleep(0) and spinning tightly. Ensure the validation references
the session_ttl parameter passed to run so callers receive a fast, explicit
failure instead of triggering the cleanup loop.
In `@src/trellis/utils/subprocess.py`:
- Around line 76-93: The async shutdown in stop_child_process_async currently
signals only the child PID; update it to mirror _stop_posix() by sending signals
to the process group on POSIX: when process.pid is set and the platform supports
process groups (e.g., os.name != "nt" or hasattr(os, "killpg")), call
os.killpg(os.getpgid(process.pid), signal.SIGTERM) instead of
process.terminate(), await process.wait() with asyncio.TimeoutError handling,
and on timeout call os.killpg(..., signal.SIGKILL); fall back to
process.terminate()/process.kill() for non-POSIX or if process.pid is missing,
and keep the existing ProcessLookupError handling. Ensure imports for os and
signal and use asyncio.TimeoutError in the except clause.
---
Nitpick comments:
In `@src/trellis/platforms/common/client/src/ClientApp.tsx`:
- Around line 28-33: The connection failure is only console.logged inside the
useEffect that calls client.connect(); update that catch handler in the
useEffect (the client.connect() promise) to call setError with a meaningful
message (including err.message or err) and clear any connecting state (e.g.,
call setIsConnecting(false) if present) so the UI is updated; ensure the same is
done for any thrown errors from client.connect() so the user sees the error and
the component stops showing a loading/connecting state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d353c9d-0b85-468f-ba1f-1c975058613c
📒 Files selected for processing (29)
.gitignoresrc/trellis/app/apploader.pysrc/trellis/bundler/build.pysrc/trellis/bundler/steps.pysrc/trellis/cli/bundle.pysrc/trellis/cli/package.pysrc/trellis/cli/run.pysrc/trellis/platforms/browser/build_steps.pysrc/trellis/platforms/browser/client/src/main.tsxsrc/trellis/platforms/common/client/src/ClientApp.tsxsrc/trellis/platforms/common/client/src/init.tssrc/trellis/platforms/common/client/src/ssr.tssrc/trellis/platforms/desktop/client/src/main.tsxsrc/trellis/platforms/server/client/src/main.tsxsrc/trellis/platforms/server/platform.pysrc/trellis/platforms/server/routes.pysrc/trellis/platforms/server/ssr.pysrc/trellis/platforms/server/ssr_renderer.pysrc/trellis/utils/subprocess.pytests/e2e/_run_test_server.pytests/py/integration/test_platform_incremental_builds.pytests/py/unit/test_apploader.pytests/py/unit/test_browser_build_steps.pytests/py/unit/test_build_steps.pytests/py/unit/test_bundler_unit.pytests/py/unit/test_ssr_bundle_step.pytests/py/unit/test_ssr_orchestrator.pytests/py/unit/test_ssr_pre_render_step.pytests/py/unit/test_step_contracts.py
✅ Files skipped from review due to trivial changes (7)
- .gitignore
- tests/py/unit/test_bundler_unit.py
- tests/py/integration/test_platform_incremental_builds.py
- tests/py/unit/test_apploader.py
- tests/py/unit/test_ssr_bundle_step.py
- tests/py/unit/test_build_steps.py
- src/trellis/platforms/server/ssr.py
🚧 Files skipped from review as they are similar to previous changes (6)
- src/trellis/cli/package.py
- src/trellis/bundler/build.py
- tests/e2e/_run_test_server.py
- src/trellis/cli/run.py
- src/trellis/platforms/browser/build_steps.py
- tests/py/unit/test_ssr_orchestrator.py
- server/main.tsx: wrap createClient in useCallback to prevent client recreation on every render (matching desktop pattern) - ssr_renderer.py: call _stop_locked() instead of self.stop() in _start_locked() to avoid deadlock on non-reentrant asyncio.Lock
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/trellis/platforms/server/platform.py (1)
196-239:⚠️ Potential issue | 🟠 MajorMake the shutdown
finallycover SSR startup too.
ssr_rendererandcleanup_taskare created before Line 233 enters thetry. If anything later in startup raises, both resources leak.♻️ Wrap resource acquisition and await task cancellation
+import contextlib @@ - session_store, ssr_renderer = await _setup_ssr( - app, - root_component=root_component, - app_wrapper=app_wrapper, - static_dir=static_dir, - session_ttl=session_ttl, - ssr_enabled=ssr, - hot_reload=hot_reload, - ) - - # Periodic cleanup of expired sessions - cleanup_task = asyncio.create_task( - _session_cleanup_loop(session_store, interval=session_ttl) - ) - - # Set up static file serving - static = static_dir or create_static_dir() - if static.exists(): - app.mount("/static", StaticFiles(directory=static), name="static") - - # Register SPA fallback for client-side routing (must be after static files) - register_spa_fallback(app) - - # Find available port if not specified - if port is None: - port = find_available_port(host=host) - - _print_startup_banner(host, port) - - config = uvicorn.Config( - app, - host=host, - port=port, - log_config=None, # Don't override logging config - log_level="warning", # Suppress uvicorn's info messages - ) - server = uvicorn.Server(config) + ssr_renderer: SSRRenderer | None = None + cleanup_task: asyncio.Task[None] | None = None try: + session_store, ssr_renderer = await _setup_ssr( + app, + root_component=root_component, + app_wrapper=app_wrapper, + static_dir=static_dir, + session_ttl=session_ttl, + ssr_enabled=ssr, + hot_reload=hot_reload, + ) + cleanup_task = asyncio.create_task( + _session_cleanup_loop(session_store, interval=session_ttl) + ) + + static = static_dir or create_static_dir() + if static.exists(): + app.mount("/static", StaticFiles(directory=static), name="static") + + register_spa_fallback(app) + + if port is None: + port = find_available_port(host=host) + + _print_startup_banner(host, port) + + config = uvicorn.Config( + app, + host=host, + port=port, + log_config=None, + log_level="warning", + ) + server = uvicorn.Server(config) await server.serve() finally: - cleanup_task.cancel() + if cleanup_task is not None: + cleanup_task.cancel() + with contextlib.suppress(asyncio.CancelledError): + await cleanup_task if ssr_renderer is not None: await ssr_renderer.stop()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/platform.py` around lines 196 - 239, The code currently creates cleanup_task and ssr_renderer (from _setup_ssr) before entering the try that awaits server. Move the try/finally to start immediately after acquiring those resources so that cleanup_task.cancel() and await ssr_renderer.stop() always run if any subsequent startup step fails; specifically wrap the block starting just after calling _setup_ssr (and including creation of cleanup_task, static mounting, register_spa_fallback, find_available_port, _print_startup_banner, uvicorn.Config/Server creation, and await server.serve()) in a try with the existing finally that cancels cleanup_task and stops ssr_renderer, ensuring you check ssr_renderer is not None before awaiting stop and handle cancellation safely.
♻️ Duplicate comments (2)
src/trellis/platforms/server/platform.py (1)
159-160:⚠️ Potential issue | 🟠 MajorValidate
session_ttlbefore starting the cleanup loop.This value becomes the sleep interval at Line 244. A direct
run(..., session_ttl=0)call will spin_session_cleanup_loop()continuously.🛡️ Fail fast here
) -> None: """Start FastAPI server with WebSocket support. @@ ssr: Enable server-side rendering (default True) session_ttl: Session time-to-live in seconds (default 300) """ + if session_ttl <= 0: + raise ValueError("session_ttl must be greater than 0") + # Start hot reload if enabled if hot_reload:Also applies to: 206-209, 241-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/platform.py` around lines 159 - 160, Validate the session_ttl parameter before starting the cleanup loop: in the entry point that accepts session_ttl (the run(...) function / Platform startup path) ensure session_ttl is a positive integer (e.g., raise ValueError or coerce to a safe minimum like 1) and reject or normalize zero/negative values so _session_cleanup_loop() never receives 0 and cannot spin continuously; update the validation in the same call site that defines the ssr: bool = True, session_ttl: int = 300 signature and any other places that start the loop so all callers (including where _session_cleanup_loop is scheduled) use the validated value.src/trellis/utils/subprocess.py (1)
76-93:⚠️ Potential issue | 🟠 MajorTerminate the whole POSIX process group in the async stop path.
SSRRendererstarts Bun in a new session, but this helper only signals the leader PID. On POSIX, any children of that sidecar survive restart/shutdown.🩹 Mirror `_stop_posix()` here
async def stop_child_process_async( process: asyncio.subprocess.Process, timeout: float = 5.0 ) -> None: @@ try: - process.terminate() + if os.name == "nt": + process.terminate() + else: + import signal # noqa: PLC0415 — only needed on POSIX + + os.killpg(process.pid, signal.SIGTERM) await asyncio.wait_for(process.wait(), timeout=timeout) except ProcessLookupError: pass except TimeoutError: try: - process.kill() + if os.name == "nt": + process.kill() + else: + import signal # noqa: PLC0415 — only needed on POSIX + + os.killpg(process.pid, signal.SIGKILL) await process.wait() - except ProcessLookupError: + except (ProcessLookupError, OSError): pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/utils/subprocess.py` around lines 76 - 93, stop_child_process_async currently only signals the leader PID so descendant processes in a POSIX session survive; on POSIX platforms, detect os.name == "posix" and if process.pid is set send the signal to the process group (use os.killpg(process.pid, signal.SIGTERM) or os.kill(-process.pid, signal.SIGTERM)) instead of process.terminate(), await wait with timeout, and on TimeoutError escalate by sending SIGKILL to the group (os.killpg/kill with SIGKILL). Keep existing ProcessLookupError handling and ensure you only use the group-kill branch on POSIX and when process.pid is present.
🧹 Nitpick comments (1)
tests/py/unit/test_ssr_orchestrator.py (1)
45-59: Avoid asserting throughorch._ssr_renderer.Line 140 reaches into a private field. Returning the mock renderer from the fixture, or adding an
# INTERNAL TEST:note, keeps this test aligned with the repo’s test contract.♻️ Small fixture tweak
-@pytest.fixture -def label_orchestrator_with_renderer(app_wrapper: tp.Any) -> SSROrchestrator: +@pytest.fixture +def label_orchestrator_with_renderer( + app_wrapper: tp.Any, +) -> tuple[SSROrchestrator, MagicMock]: @@ - return SSROrchestrator( + orchestrator = SSROrchestrator( root_component=App, app_wrapper=app_wrapper, session_store=store, ssr_renderer=renderer, ) + return orchestrator, renderer @@ - orch = label_orchestrator_with_renderer - renderer = orch._ssr_renderer - assert renderer is not None + orch, renderer = label_orchestrator_with_rendererAs per coding guidelines, "Test via public API, not
_privateattributes; document with# INTERNAL TEST: <reason>comment if testing internals is necessary".Also applies to: 135-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/py/unit/test_ssr_orchestrator.py` around lines 45 - 59, The test fixture label_orchestrator_with_renderer currently reaches into the private field orch._ssr_renderer to assert behavior; instead modify the fixture to return the mock renderer alongside/within the SSROrchestrator (e.g., return a tuple (orchestrator, renderer) or attach the mock to a public test-only property) so tests use the public API, or if you must inspect internals add a clear "# INTERNAL TEST: accessing _ssr_renderer to verify mock renderer from fixture" comment next to the SSROrchestrator creation; update all tests that currently reference _ssr_renderer to use the returned renderer or acknowledge the internal access comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/trellis/platforms/server/routes.py`:
- Around line 27-35: _is_document_request currently treats any request with
"text/html" in the Accept header as a document navigation; update it to only
return True for browser navigations that are safe for SPA fallback by also
checking the HTTP method: require request.method to be "GET" or "HEAD" in
addition to Accept containing "text/html". Modify the
_is_document_request(request: Request) function to inspect request.method (use
request.method.upper()) and only return True when method is "GET" or "HEAD" and
Accept includes "text/html" so POST/PUT/DELETE requests no longer produce the
SPA HTML fallback.
---
Outside diff comments:
In `@src/trellis/platforms/server/platform.py`:
- Around line 196-239: The code currently creates cleanup_task and ssr_renderer
(from _setup_ssr) before entering the try that awaits server. Move the
try/finally to start immediately after acquiring those resources so that
cleanup_task.cancel() and await ssr_renderer.stop() always run if any subsequent
startup step fails; specifically wrap the block starting just after calling
_setup_ssr (and including creation of cleanup_task, static mounting,
register_spa_fallback, find_available_port, _print_startup_banner,
uvicorn.Config/Server creation, and await server.serve()) in a try with the
existing finally that cancels cleanup_task and stops ssr_renderer, ensuring you
check ssr_renderer is not None before awaiting stop and handle cancellation
safely.
---
Duplicate comments:
In `@src/trellis/platforms/server/platform.py`:
- Around line 159-160: Validate the session_ttl parameter before starting the
cleanup loop: in the entry point that accepts session_ttl (the run(...) function
/ Platform startup path) ensure session_ttl is a positive integer (e.g., raise
ValueError or coerce to a safe minimum like 1) and reject or normalize
zero/negative values so _session_cleanup_loop() never receives 0 and cannot spin
continuously; update the validation in the same call site that defines the ssr:
bool = True, session_ttl: int = 300 signature and any other places that start
the loop so all callers (including where _session_cleanup_loop is scheduled) use
the validated value.
In `@src/trellis/utils/subprocess.py`:
- Around line 76-93: stop_child_process_async currently only signals the leader
PID so descendant processes in a POSIX session survive; on POSIX platforms,
detect os.name == "posix" and if process.pid is set send the signal to the
process group (use os.killpg(process.pid, signal.SIGTERM) or
os.kill(-process.pid, signal.SIGTERM)) instead of process.terminate(), await
wait with timeout, and on TimeoutError escalate by sending SIGKILL to the group
(os.killpg/kill with SIGKILL). Keep existing ProcessLookupError handling and
ensure you only use the group-kill branch on POSIX and when process.pid is
present.
---
Nitpick comments:
In `@tests/py/unit/test_ssr_orchestrator.py`:
- Around line 45-59: The test fixture label_orchestrator_with_renderer currently
reaches into the private field orch._ssr_renderer to assert behavior; instead
modify the fixture to return the mock renderer alongside/within the
SSROrchestrator (e.g., return a tuple (orchestrator, renderer) or attach the
mock to a public test-only property) so tests use the public API, or if you must
inspect internals add a clear "# INTERNAL TEST: accessing _ssr_renderer to
verify mock renderer from fixture" comment next to the SSROrchestrator creation;
update all tests that currently reference _ssr_renderer to use the returned
renderer or acknowledge the internal access comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f401b2f8-e02b-4762-9b62-8245eb1a7750
📒 Files selected for processing (29)
.gitignoresrc/trellis/app/apploader.pysrc/trellis/bundler/build.pysrc/trellis/bundler/steps.pysrc/trellis/cli/bundle.pysrc/trellis/cli/package.pysrc/trellis/cli/run.pysrc/trellis/platforms/browser/build_steps.pysrc/trellis/platforms/browser/client/src/main.tsxsrc/trellis/platforms/common/client/src/ClientApp.tsxsrc/trellis/platforms/common/client/src/init.tssrc/trellis/platforms/common/client/src/ssr.tssrc/trellis/platforms/desktop/client/src/main.tsxsrc/trellis/platforms/server/client/src/main.tsxsrc/trellis/platforms/server/platform.pysrc/trellis/platforms/server/routes.pysrc/trellis/platforms/server/ssr.pysrc/trellis/platforms/server/ssr_renderer.pysrc/trellis/utils/subprocess.pytests/e2e/_run_test_server.pytests/py/integration/test_platform_incremental_builds.pytests/py/unit/test_apploader.pytests/py/unit/test_browser_build_steps.pytests/py/unit/test_build_steps.pytests/py/unit/test_bundler_unit.pytests/py/unit/test_ssr_bundle_step.pytests/py/unit/test_ssr_orchestrator.pytests/py/unit/test_ssr_pre_render_step.pytests/py/unit/test_step_contracts.py
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- tests/py/unit/test_bundler_unit.py
- tests/py/unit/test_ssr_bundle_step.py
- src/trellis/platforms/server/ssr.py
🚧 Files skipped from review as they are similar to previous changes (8)
- src/trellis/cli/package.py
- src/trellis/cli/run.py
- src/trellis/bundler/build.py
- tests/py/integration/test_platform_incremental_builds.py
- tests/e2e/_run_test_server.py
- src/trellis/platforms/browser/build_steps.py
- src/trellis/platforms/browser/client/src/main.tsx
- tests/py/unit/test_ssr_pre_render_step.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/trellis/platforms/server/routes.py (1)
63-76: Consider cachingindex.htmlcontent to avoid repeated disk reads.
get_index_html()is called on every request and reads from disk synchronously. For high-traffic scenarios, caching the content in memory would eliminate repeated I/O. This could use a simple module-level cache with invalidation on hot reload.♻️ Example caching approach
_index_html_cache: str | None = None def get_index_html() -> str: global _index_html_cache if _index_html_cache is None: index_path = create_static_dir() / "index.html" _index_html_cache = index_path.read_text() return _index_html_cache def invalidate_index_cache() -> None: global _index_html_cache _index_html_cache = NoneWire
invalidate_index_cache()into the hot-reload callback alongside SSR cache invalidation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trellis/platforms/server/routes.py` around lines 63 - 76, The code repeatedly calls get_index_html() per request causing synchronous disk I/O; add a module-level cache (e.g., _index_html_cache) inside the module that get_index_html() uses to return cached content on subsequent calls, and implement invalidate_index_cache() to clear the cache; update any hot-reload or SSR invalidation hooks (where _get_ssr_orchestrator or SSR orchestration invalidation occurs) to call invalidate_index_cache() so updated index.html is picked up on reload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/trellis/platforms/server/routes.py`:
- Around line 63-76: The code repeatedly calls get_index_html() per request
causing synchronous disk I/O; add a module-level cache (e.g., _index_html_cache)
inside the module that get_index_html() uses to return cached content on
subsequent calls, and implement invalidate_index_cache() to clear the cache;
update any hot-reload or SSR invalidation hooks (where _get_ssr_orchestrator or
SSR orchestration invalidation occurs) to call invalidate_index_cache() so
updated index.html is picked up on reload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53217d7e-0afe-4ff4-898c-4a446c5e6e52
📒 Files selected for processing (1)
src/trellis/platforms/server/routes.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
render_for_ssr()renders the component tree but defers mount hooks;SessionStoreholds the session for WebSocket resumption with a 30s TTLSSRRenderer) runsrenderToString()over a Unix socket to produce HTML from the serialized element treeSSROrchestratorcoordinates rendering, session storage, dehydration script injection, and per-route+theme HTML cachingwindow.__TRELLIS_SSR__, pre-populates the store with patches, and useshydrateRoot()for seamless hydrationssrconfig option (defaulttrue) controls the feature; graceful fallback to CSR when the renderer is unavailableSSRBundleBuildStepproduces a Node-targeted ESM bundle from a newssr-entry.tsxadd_on_reload_callback()Test plan
pixi run test)pixi run ci— formatting, lint, mypy, tests)--force-build, verify SSR HTML in page sourcessr=falseconfig falls back to CSRSummary by CodeRabbit