feat(builder): auto-discover UI modules and add ui:register#1030
Open
Rod-Christensen wants to merge 7 commits into
Open
feat(builder): auto-discover UI modules and add ui:register#1030Rod-Christensen wants to merge 7 commits into
Rod-Christensen wants to merge 7 commits into
Conversation
…otstrap logging Cloud URI: - Remove .config file; all build-time env vars now come from .env - VSCode cloud mode: ignore stale hostUrl from settings, use build-time ROCKETRIDE_URI or default to https://api.rocketride.ai - rsbuild: default ROCKETRIDE_URI to https://api.rocketride.ai so the CloudPanel probe fires correctly on first boot - Update docs and SDK references from cloud.rocketride.ai to api.rocketride.ai Replica Manager: - Restore drain-time scaling constants (CONST_SCALE_UP_DRAIN_TIME_S etc.) that were renamed to queue-depth constants in 51f3b0d but never consumed; replica_manager.py still imports the old names and would crash on import Bootstrap debugging: - Add console.log tracing to shell-ui bootstrap and finishConnect to diagnose first-login hang on cloud.rocketride.ai Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace hardcoded app lists in ui:build and ui:clean with dynamic registry lookup — any registered *-ui module (OSS or overlay) is picked up automatically without maintaining a manual list. Add ui:register aggregate task that writes all app manifest entries into apps.json without bundling. Used by the SaaS engine CI to generate the complete app catalog for DB seeding without the cost of a full UI build. Add no-op :register actions to chat-ui and dropper-ui (hand-written modules that don't use createAppModule and have no apps.json entry). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR migrates documented and configured RocketRide endpoints to api.rocketride.ai, adds per-UI register stubs and dynamic UI task orchestration, increases bootstrap/connection logging, refactors model auto-scaling constants, and separates shell vs apps static asset routing with corresponding build/path updates. ChangesAPI Endpoint Hostname Migration
UI Module Registration and Dynamic Task Orchestration
Bootstrap and Connection Diagnostics
Model Auto-Scaling Configuration
Static Asset Routing Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
No description provided. |
![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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/client-python/src/rocketride/core/constants.py`:
- Around line 64-66: CONST_DEFAULT_WEB_CLOUD currently hard-codes the port
suffix ":80", causing inconsistent behavior versus other clients; update the
constant value (CONST_DEFAULT_WEB_CLOUD) to remove the explicit port so it
becomes 'https://api.rocketride.ai', ensuring RocketRideClient() defaults match
the docs/tests/other SDKs and will use the standard HTTPS port.
In `@packages/client-typescript/src/cli/rocketride.ts`:
- Line 44: The comment claiming ROCKETRIDE_URI defaults to
"wss://api.rocketride.ai" is stale; update the docstring in
packages/client-typescript/src/cli/rocketride.ts to reflect the runtime behavior
by stating that the CLI uses process.env.ROCKETRIDE_URI or falls back to
CONST_DEFAULT_WEB_LOCAL (the CONST_DEFAULT_WEB_LOCAL constant) as the default.
Locate the comment mentioning ROCKETRIDE_URI in rocketride.ts and replace the
fixed URI text with a note that the default is process.env.ROCKETRIDE_URI ||
CONST_DEFAULT_WEB_LOCAL so the docs match the actual logic that uses
CONST_DEFAULT_WEB_LOCAL at runtime.
In `@packages/client-typescript/src/client/constants.ts`:
- Around line 52-55: The default cloud endpoint CONST_DEFAULT_WEB_CLOUD still
includes the explicit :80 port ('https://api.rocketride.ai:80') which diverges
from the rest of the codebase; update the constant CONST_DEFAULT_WEB_CLOUD in
packages/client-typescript/src/client/constants.ts to use the portless host
('https://api.rocketride.ai') so new RocketRideClient() uses the same default
URI as tests, docs, and VS Code defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e7d5cfe5-4826-43b2-8a05-d93dff08ea3e
📒 Files selected for processing (21)
.configapps/chat-ui/scripts/tasks.jsapps/dropper-ui/scripts/tasks.jsapps/shell-ui/src/components/layout/Shell.tsxapps/shell-ui/src/connection/connection.tsapps/vscode/rsbuild.config.mjsapps/vscode/src/config.tsdocs/README-python-client.mddocs/README-typescript-client.mddocs/agents/ROCKETRIDE_PIPELINE_RULES.mddocs/agents/ROCKETRIDE_QUICKSTART.mddocs/agents/ROCKETRIDE_python_API.mddocs/agents/ROCKETRIDE_typescript_API.mdpackages/ai/src/ai/constants.pypackages/client-python/src/rocketride/core/constants.pypackages/client-python/tests/RocketRideClient_test.pypackages/client-typescript/src/cli/rocketride.tspackages/client-typescript/src/client/client.tspackages/client-typescript/src/client/constants.tspackages/client-typescript/tests/RocketRideClient.test.tsscripts/tasks.js
💤 Files with no reviewable changes (1)
- .config
Comment on lines
64
to
+66
| # Default cloud RocketRide service endpoint URL | ||
| # This points to the production Enterprise as a Service (EaaS) instance | ||
| CONST_DEFAULT_WEB_CLOUD = 'https://cloud.rocketride.ai:80' | ||
| CONST_DEFAULT_WEB_CLOUD = 'https://api.rocketride.ai:80' |
Contributor
There was a problem hiding this comment.
Drop the explicit :80 from the cloud default.
Every other updated surface in this PR now points at https://api.rocketride.ai without a port, but the Python SDK default still hard-codes :80. That means RocketRideClient() with no explicit uri will behave differently from the docs, tests, and VS Code client, and it can fail outright if the new host is only serving TLS on the default HTTPS port.
Suggested fix
-CONST_DEFAULT_WEB_CLOUD = 'https://api.rocketride.ai:80'
+CONST_DEFAULT_WEB_CLOUD = 'https://api.rocketride.ai'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Default cloud RocketRide service endpoint URL | |
| # This points to the production Enterprise as a Service (EaaS) instance | |
| CONST_DEFAULT_WEB_CLOUD = 'https://cloud.rocketride.ai:80' | |
| CONST_DEFAULT_WEB_CLOUD = 'https://api.rocketride.ai:80' | |
| # Default cloud RocketRide service endpoint URL | |
| # This points to the production Enterprise as a Service (EaaS) instance | |
| CONST_DEFAULT_WEB_CLOUD = 'https://api.rocketride.ai' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/client-python/src/rocketride/core/constants.py` around lines 64 -
66, CONST_DEFAULT_WEB_CLOUD currently hard-codes the port suffix ":80", causing
inconsistent behavior versus other clients; update the constant value
(CONST_DEFAULT_WEB_CLOUD) to remove the explicit port so it becomes
'https://api.rocketride.ai', ensuring RocketRideClient() defaults match the
docs/tests/other SDKs and will use the standard HTTPS port.
| * The client supports configuration via .env file with the following variables: | ||
| * - ROCKETRIDE_APIKEY: Your RocketRide API key (required for authentication) | ||
| * - ROCKETRIDE_URI: The RocketRide server URI (defaults to wss://cloud.rocketride.ai) | ||
| * - ROCKETRIDE_URI: The RocketRide server URI (defaults to wss://api.rocketride.ai) |
Contributor
There was a problem hiding this comment.
Default URI comment does not match runtime default source.
Line 44 states a fixed default (wss://api.rocketride.ai), but the CLI actually defaults to process.env.ROCKETRIDE_URI || CONST_DEFAULT_WEB_LOCAL (Line 874). Please align the comment with runtime behavior.
Suggested doc fix
- * - ROCKETRIDE_URI: The RocketRide server URI (defaults to wss://api.rocketride.ai)
+ * - ROCKETRIDE_URI: The RocketRide server URI (defaults to ROCKETRIDE_URI env var, otherwise CONST_DEFAULT_WEB_LOCAL)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * - ROCKETRIDE_URI: The RocketRide server URI (defaults to wss://api.rocketride.ai) | |
| * - ROCKETRIDE_URI: The RocketRide server URI (defaults to ROCKETRIDE_URI env var, otherwise CONST_DEFAULT_WEB_LOCAL) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/client-typescript/src/cli/rocketride.ts` at line 44, The comment
claiming ROCKETRIDE_URI defaults to "wss://api.rocketride.ai" is stale; update
the docstring in packages/client-typescript/src/cli/rocketride.ts to reflect the
runtime behavior by stating that the CLI uses process.env.ROCKETRIDE_URI or
falls back to CONST_DEFAULT_WEB_LOCAL (the CONST_DEFAULT_WEB_LOCAL constant) as
the default. Locate the comment mentioning ROCKETRIDE_URI in rocketride.ts and
replace the fixed URI text with a note that the default is
process.env.ROCKETRIDE_URI || CONST_DEFAULT_WEB_LOCAL so the docs match the
actual logic that uses CONST_DEFAULT_WEB_LOCAL at runtime.
Comment on lines
52
to
+55
| * Default cloud RocketRide service endpoint URL. | ||
| * Used when no custom URI is provided in the client configuration. | ||
| */ | ||
| export const CONST_DEFAULT_WEB_CLOUD = 'https://cloud.rocketride.ai:80'; | ||
| export const CONST_DEFAULT_WEB_CLOUD = 'https://api.rocketride.ai:80'; |
Contributor
There was a problem hiding this comment.
Align the TypeScript SDK default with the rest of the new endpoint.
This constant still hard-codes https://api.rocketride.ai:80, while the updated tests, docs, and VS Code defaults all use the portless host. That leaves new RocketRideClient() on a different default URI than every other consumer in this PR.
Suggested fix
-export const CONST_DEFAULT_WEB_CLOUD = 'https://api.rocketride.ai:80';
+export const CONST_DEFAULT_WEB_CLOUD = 'https://api.rocketride.ai';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * Default cloud RocketRide service endpoint URL. | |
| * Used when no custom URI is provided in the client configuration. | |
| */ | |
| export const CONST_DEFAULT_WEB_CLOUD = 'https://cloud.rocketride.ai:80'; | |
| export const CONST_DEFAULT_WEB_CLOUD = 'https://api.rocketride.ai:80'; | |
| * Default cloud RocketRide service endpoint URL. | |
| * Used when no custom URI is provided in the client configuration. | |
| */ | |
| export const CONST_DEFAULT_WEB_CLOUD = 'https://api.rocketride.ai'; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/client-typescript/src/client/constants.ts` around lines 52 - 55, The
default cloud endpoint CONST_DEFAULT_WEB_CLOUD still includes the explicit :80
port ('https://api.rocketride.ai:80') which diverges from the rest of the
codebase; update the constant CONST_DEFAULT_WEB_CLOUD in
packages/client-typescript/src/client/constants.ts to use the portless host
('https://api.rocketride.ai') so new RocketRideClient() uses the same default
URI as tests, docs, and VS Code defaults.
Add a console.log in registerAndMapApps() that prints each Module Federation remote's moduleId and entry URL right after registerRemotes() is called. This makes it easy to verify which app bundles the shell is wiring up and where they resolve to. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/shell-ui/src/lib/appLoader.ts`:
- Around line 89-93: The unconditional console.log that prints MF remote entry
URLs (the block logging '[shell-ui] MF remotes registered:' using
validApps.map(...)) should only run in development; wrap that log in a
development-environment guard (e.g., check process.env.NODE_ENV !== 'production'
or import.meta.env.DEV where available) so it does not execute in production
builds and thereby avoids noisy/secret-leaking logs in browser consoles. Ensure
the guard surrounds the console.log call that references validApps and leave the
rest of appLoader initialization intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 89b573fc-8673-41a6-9fef-e0719c32dbb9
📒 Files selected for processing (1)
apps/shell-ui/src/lib/appLoader.ts
Comment on lines
+89
to
+93
| // Log registered MF modules and their remote entry paths | ||
| console.log( | ||
| '[shell-ui] MF remotes registered:', | ||
| validApps.map((a) => `${a.moduleId} → ${a.entry}`), | ||
| ); |
Contributor
There was a problem hiding this comment.
Gate this debug log behind a development flag.
This emits remote entry URLs unconditionally in production, which is noisy and can leak deployment topology details in browser consoles.
Suggested change
- // Log registered MF modules and their remote entry paths
- console.log(
- '[shell-ui] MF remotes registered:',
- validApps.map((a) => `${a.moduleId} → ${a.entry}`),
- );
+ // Log registered MF modules and their remote entry paths (dev-only)
+ if (import.meta.env.DEV) {
+ console.log(
+ '[shell-ui] MF remotes registered:',
+ validApps.map((a) => `${a.moduleId} → ${a.entry}`),
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Log registered MF modules and their remote entry paths | |
| console.log( | |
| '[shell-ui] MF remotes registered:', | |
| validApps.map((a) => `${a.moduleId} → ${a.entry}`), | |
| ); | |
| // Log registered MF modules and their remote entry paths (dev-only) | |
| if (import.meta.env.DEV) { | |
| console.log( | |
| '[shell-ui] MF remotes registered:', | |
| validApps.map((a) => `${a.moduleId} → ${a.entry}`), | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/shell-ui/src/lib/appLoader.ts` around lines 89 - 93, The unconditional
console.log that prints MF remote entry URLs (the block logging '[shell-ui] MF
remotes registered:' using validApps.map(...)) should only run in development;
wrap that log in a development-environment guard (e.g., check
process.env.NODE_ENV !== 'production' or import.meta.env.DEV where available) so
it does not execute in production builds and thereby avoids noisy/secret-leaking
logs in browser consoles. Ensure the guard surrounds the console.log call that
references validApps and leave the rest of appLoader initialization intact.
The /shell/apps/ path for Module Federation remote bundles created
confusion and bugs: the shell dev server's publicDir pointed at
dist/server/static/ expecting bundles at static/shell/apps/, but the
builder output them to build/apps/. The extra /shell/ nesting also
forced the CI workflow to maintain duplicate copy targets.
This commit gives app bundles their own top-level /apps/ route,
separating them from the shell's own assets (/shell/ for JS/CSS/themes).
Build system:
- registerApp.js: entry, icon, and readme URLs now use /${APPS_BASE}/
instead of /shell/${APPS_BASE}/ (e.g. /apps/home-ui/remoteEntry.js)
- appModule.js: copy destination changed from
dist/server/static/shell/apps/<name> to dist/server/static/apps/<name>
Server:
- shell.py: added _apps_root (static/apps/) and new apps_static()
handler that serves MF bundles with path-traversal protection and
404 responses (no SPA fallback — these are JS/CSS assets, not routes)
- shell/__init__.py: registered GET /apps/{file_path:path} route
alongside the existing /shell/ route
Shell host (dev):
- rsbuild.config.mts: dev publicDir now serves from build/ (via
ROCKETRIDE_BUILD_ROOT) instead of dist/server/static/, so /apps/*
resolves directly to the builder output at build/apps/*
- bootstrap.tsx: import Shell and AppManifestEntry from relative paths
instead of bare 'shell-ui' — MF intercepts bare imports and tries to
load from the share scope, but the host hasn't registered its own
factory yet, causing an undefined factory error (RUNTIME-006)
Types:
- client.ts: updated AppManifestEntry.icon docstring example path
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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)
scripts/lib/registerApp.js (1)
50-50:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix:
APPS_BASE_URLcan’t safely change because runtime serving is hardcoded to/apps
APPS_BASEis derived fromprocess.env.APPS_BASE_URL ?? 'apps'and is used to generateapps.jsonURLs (e.g.,icon,readme,entry). That part is configurable here, but the rest of the system assumesappsis always the URL/path segment:
- Bundles are built into/copy to
.../server/static/apps/<name>(viascripts/lib/appModule.js), not a configurable base.- The server route is hardcoded to
GET /apps/{file_path:path}andapps_staticresolves paths underapps/(inpackages/ai/src/ai/modules/shell/__init__.pyandshell.py).- Repo-wide search shows
APPS_BASE_URLis only referenced inscripts/lib/registerApp.js(no server/build consumer honors it).If
APPS_BASE_URLis set to anything other than the default (apps), the URLs written toapps.jsonwon’t match the server route or bundle output location, breaking remote loading—make the base a single source of truth across build + server, or remove the env override and hardcode'apps'consistently.const APPS_BASE = process.env.APPS_BASE_URL ?? 'apps';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/lib/registerApp.js` at line 50, APPS_BASE (const APPS_BASE in scripts/lib/registerApp.js) is the only place reading process.env.APPS_BASE_URL but the build output (scripts/lib/appModule.js) and server routes (apps_static, GET /apps/{file_path:path} in packages/ai) are hardcoded to the 'apps' path, so either remove the env override or centralize the base into one source of truth; update scripts/lib/registerApp.js to hardcode APPS_BASE = 'apps' (or read from a shared config constant used by appModule.js and the server) and ensure apps.json URL generation (icon, readme, entry) uses that same shared constant so bundle output and server routes match the generated URLs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/shell-ui/rsbuild.config.mts`:
- Line 90: The code uses path.join(process.env.ROCKETRIDE_BUILD_ROOT ??
path.resolve(__dirname, '../../build')) which leaves a relative env value
un-resolved; change this to resolve the env var explicitly so both branches
produce absolute paths — e.g. replace the path.join(...) usage for
ROCKETRIDE_BUILD_ROOT with path.resolve(process.env.ROCKETRIDE_BUILD_ROOT ??
path.resolve(__dirname, '../../build')) (or drop the redundant path.join and
call path.resolve on the env value), ensuring the publicDir value is always an
absolute path; update the expression referencing
process.env.ROCKETRIDE_BUILD_ROOT in rsbuild.config.mts accordingly.
In `@packages/ai/src/ai/modules/shell/shell.py`:
- Around line 151-152: The docstring's Raises section incorrectly mentions a 503
for a missing apps dir that the function does not raise; update the docstring
for the apps_static function to only document the actual exception
(HTTPException: 404 if file not found) and remove the "503 if apps dir missing"
clause (or alternatively adjust to reflect any real behavior if you change
code), ensuring the Raises block matches the implementation and referencing the
apps_static function (compare behavior with shell_static if needed).
---
Outside diff comments:
In `@scripts/lib/registerApp.js`:
- Line 50: APPS_BASE (const APPS_BASE in scripts/lib/registerApp.js) is the only
place reading process.env.APPS_BASE_URL but the build output
(scripts/lib/appModule.js) and server routes (apps_static, GET
/apps/{file_path:path} in packages/ai) are hardcoded to the 'apps' path, so
either remove the env override or centralize the base into one source of truth;
update scripts/lib/registerApp.js to hardcode APPS_BASE = 'apps' (or read from a
shared config constant used by appModule.js and the server) and ensure apps.json
URL generation (icon, readme, entry) uses that same shared constant so bundle
output and server routes match the generated URLs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5070c6b1-407f-4e5c-bc03-ef88e4f99830
📒 Files selected for processing (7)
apps/shell-ui/rsbuild.config.mtsapps/shell-ui/src/bootstrap.tsxpackages/ai/src/ai/modules/shell/__init__.pypackages/ai/src/ai/modules/shell/shell.pypackages/client-typescript/src/client/types/client.tsscripts/lib/appModule.jsscripts/lib/registerApp.js
| { name: path.join(process.env.ROCKETRIDE_DIST_ROOT || path.resolve(__dirname, '../../dist'), 'server', 'static'), watch: false }, | ||
| // Serve build/ so /apps/* resolves to built MF remote bundles | ||
| // and /apps.json resolves to the generated app manifest. | ||
| { name: path.join(process.env.ROCKETRIDE_BUILD_ROOT ?? path.resolve(__dirname, '../../build')), watch: false }, |
Contributor
There was a problem hiding this comment.
Inconsistent absolute/relative handling for ROCKETRIDE_BUILD_ROOT.
The fallback uses path.resolve(__dirname, '../../build') (absolute), but the env branch passes process.env.ROCKETRIDE_BUILD_ROOT through path.join(...) with a single argument, which only normalizes and leaves a relative value relative. If ROCKETRIDE_BUILD_ROOT is set to a relative path, publicDir resolution may differ from intent. Consider resolving the env value too and dropping the redundant single-arg path.join.
Proposed fix
- { name: path.join(process.env.ROCKETRIDE_BUILD_ROOT ?? path.resolve(__dirname, '../../build')), watch: false },
+ { name: path.resolve(process.env.ROCKETRIDE_BUILD_ROOT ?? path.resolve(__dirname, '../../build')), watch: false },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| { name: path.join(process.env.ROCKETRIDE_BUILD_ROOT ?? path.resolve(__dirname, '../../build')), watch: false }, | |
| { name: path.resolve(process.env.ROCKETRIDE_BUILD_ROOT ?? path.resolve(__dirname, '../../build')), watch: false }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/shell-ui/rsbuild.config.mts` at line 90, The code uses
path.join(process.env.ROCKETRIDE_BUILD_ROOT ?? path.resolve(__dirname,
'../../build')) which leaves a relative env value un-resolved; change this to
resolve the env var explicitly so both branches produce absolute paths — e.g.
replace the path.join(...) usage for ROCKETRIDE_BUILD_ROOT with
path.resolve(process.env.ROCKETRIDE_BUILD_ROOT ?? path.resolve(__dirname,
'../../build')) (or drop the redundant path.join and call path.resolve on the
env value), ensuring the publicDir value is always an absolute path; update the
expression referencing process.env.ROCKETRIDE_BUILD_ROOT in rsbuild.config.mts
accordingly.
Comment on lines
+151
to
+152
| Raises: | ||
| HTTPException: 404 if file not found, 503 if apps dir missing. |
Contributor
There was a problem hiding this comment.
Docstring lists a 503 that apps_static never raises.
The Raises section mentions "503 if apps dir missing", but the implementation only raises 404. Unlike shell_static, there is no 503 path here. Update the docstring to reflect the actual behavior.
Proposed fix
Raises:
- HTTPException: 404 if file not found, 503 if apps dir missing.
+ HTTPException: 404 if the file is not found.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Raises: | |
| HTTPException: 404 if file not found, 503 if apps dir missing. | |
| Raises: | |
| HTTPException: 404 if the file is not found. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ai/src/ai/modules/shell/shell.py` around lines 151 - 152, The
docstring's Raises section incorrectly mentions a 503 for a missing apps dir
that the function does not raise; update the docstring for the apps_static
function to only document the actual exception (HTTPException: 404 if file not
found) and remove the "503 if apps dir missing" clause (or alternatively adjust
to reflect any real behavior if you change code), ensuring the Raises block
matches the implementation and referencing the apps_static function (compare
behavior with shell_static if needed).
The .config file was accidentally removed in a previous commit. Without it, getenv() returns empty strings for RR_ZITADEL_URL, RR_ZITADEL_CLIENT_ID, and RR_STRIPE_PUBLISHABLE_KEY unless the developer has a local .env that sets them — causing "Zitadel not configured" in shell-ui and breaking OAuth login. This file provides safe-to-commit public defaults (OAuth client IDs, Zitadel issuer URL, Stripe publishable key, localhost server URI) that getenv() loads first, with .env layered on top as overrides. CI also relies on .config for the base values before writing its own .env. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
After auth, Shell.tsx called registerAndMapApps() with only the user's desktop apps (ConnectResult.apps), which invoked registerRemotes() with force:true — wiping the full probe catalog. Apps like home-ui that were registered from the probe but not in the user's desktop list became unloadable, causing the shell to hang on "Loading...". MF remote registration (moduleId → remoteEntry URL) only needs to happen once from the probe. Post-auth, the URLs don't change — the shell only needs to overlay desktop metadata (appStatus, onDesktop) onto the existing probe entries for UI display purposes. - Remove registerAndMapApps import from Shell.tsx (no longer used) - Replace the apps useMemo with a simple metadata overlay that merges desktop fields onto probe entries without touching MF registration Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…s + fix workspace seed state Tokenizer: AutoTokenizer.from_pretrained() tries fast (Rust) tokenizer first; models like Davlan/xlm-roberta-base-ner-hrl use SentencePiece which can't auto-convert to Rust, causing persistent load failures in chaos tests. Now retries with use_fast=False before falling back to AutoProcessor. Workspace: set seeded=true on all load paths so downstream consumers don't block waiting for seed completion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
ui:buildandui:cleanwith dynamic registry lookup — any*-uimodule (OSS or overlay) is picked up automaticallyui:registeraggregate task that writes all app manifest entries intoapps.jsonwithout bundling — used by SaaS engine CI for DB seeding:registeractions tochat-uianddropper-ui(hand-written modules withoutcreateAppModule)Test plan
./builder ui:register --verbosewrites all apps (OSS + SaaS) tobuild/apps.json./builder ui:buildstill builds shell + all remotes./builder ui:cleanstill cleans all app artifacts🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores