Improve workbench services: fetch timeout, fallback template, fix CLI tests and duplication#1487
Improve workbench services: fetch timeout, fallback template, fix CLI tests and duplication#1487Copilot wants to merge 3 commits intofeature/distributed-demofrom
Conversation
…and integrate PR #1484 changes - Add AbortSignal.timeout(8000) to fetchWorkbenchServices thunk - Create client/src/utils/services.json as fallback template - On fetch failure, load fallback and replace 'username' in VNC endpoint - Change thunk param to { url, username } for fallback username use - Fix CLI test expectations to use intocps/workspace:latest" Co-authored-by: prasadtalasila <9206466+prasadtalasila@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Please upload reports for the commit 3f898e9 to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## feature/distributed-demo #1487 +/- ##
============================================================
+ Coverage 96.33% 96.96% +0.63%
============================================================
Files 124 5 -119
Lines 3633 264 -3369
Branches 579 0 -579
============================================================
- Hits 3500 256 -3244
+ Misses 131 8 -123
+ Partials 2 0 -2 see 129 files with indirect coverage changes
🚀 New features to boost your workflow:
|
- Extract assertFallbackBehavior helper to eliminate repeated store creation, dispatch, and assertion patterns across three fallback test cases - Apply prettier formatting (yarn format) to workbenchSlice.test.ts - All 544 unit tests and 89 integration tests pass Co-authored-by: prasadtalasila <9206466+prasadtalasila@users.noreply.github.com>
|
There was a problem hiding this comment.
Pull request overview
This PR improves the reliability of Workbench tool links by fetching /services with a timeout and falling back to a static template when the dynamic endpoint fails, while also aligning Docker image names across compose templates and fixing/refactoring related tests.
Changes:
- Add
workbenchRedux slice +fetchWorkbenchServicesthunk with 8s timeout and static JSON fallback. - Update Workbench link construction to derive tool links from Redux services instead of env vars; keep preview links in env.
- Rename workspace Docker image across compose/templates and update CLI/client tests accordingly.
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| docker/compose.dev.yml | Update workspace image name used for dev users. |
| deploy/docker/compose.server.yml | Update workspace image name for server deployment. |
| deploy/docker/compose.server.secure.yml | Update workspace image name for secure server deployment. |
| deploy/docker/compose.local.yml | Update workspace image name for local deployment. |
| deploy/docker/compose.local.secure.yml | Update workspace image name for secure local deployment. |
| client/test/unit/util/envUtil.test.ts | Adjust tests to validate workbench links from Redux services and preview links from env. |
| client/test/unit/store/workbenchSlice.test.ts | Add unit tests for new workbench slice + fallback behavior. |
| client/test/unit/routes/Workbench.test.tsx | Mock Redux hooks to accommodate new Workbench behavior. |
| client/test/unit/page/Config.test.tsx | Update expected config keys shown after env var removals. |
| client/test/integration/Routes/Workbench.test.tsx | Update integration test to seed services into store and validate rendered tool buttons. |
| client/test/mocks/global_mocks.tsx | Remove deprecated workbench link env vars from global test env. |
| client/src/utils/services.json | Add static fallback services template used on fetch failure/timeout. |
| client/src/util/envUtil.ts | Build workbench tool links from Redux services; keep preview links from env. |
| client/src/util/configUtil.ts | Remove deprecated workbench tool env vars from validation key list. |
| client/src/store/workbench.slice.ts | Introduce workbench slice with Zod validation, timeout, and fallback services. |
| client/src/store/store.ts | Register workbench reducer and export AppDispatch. |
| client/src/route/workbench/Workbench.tsx | Dispatch fetchWorkbenchServices on initial Workbench load. |
| client/env.d.ts | Remove deprecated workbench tool env vars from type declarations. |
| client/config/test.js | Remove deprecated workbench tool env vars from test config. |
| client/config/prod.js | Remove deprecated workbench tool env vars from prod config. |
| client/config/local.js | Remove deprecated workbench tool env vars from local config. |
| client/config/dev.js | Remove deprecated workbench tool env vars from dev config. |
| cli/users.server.yml | Update workspace image name in CLI server template. |
| cli/users.server.secure.yml | Update workspace image name in CLI secure server template. |
| cli/users.local.yml | Update workspace image name in CLI local template. |
| cli/tests/test_utils.py | Update expected image name in CLI YAML/template tests. |
| cli/tests/data/compose.users.test.yml | Update workspace image name in CLI test compose input. |
| cli/tests/data/compose.users.exp.yml | Update workspace image name in CLI test compose expected output. |
Comments suppressed due to low confidence (2)
client/test/integration/Routes/Workbench.test.tsx:80
- In this
waitForblock,testTool(...)is anasyncfunction but its Promise is not returned/awaited. Assertion failures insidetestToolwill become a rejected Promise that may not fail the test reliably (andwaitForwill likely pass immediately). Either maketestToolsynchronous, or return/await the Promise from thewaitForcallback (e.g.,await waitFor(async () => { await testTool(...) })orawait waitFor(() => testTool(...))).
await waitFor(() => {
testTool(desktopLabel, 'Desktop');
});
client/src/route/workbench/Workbench.tsx:37
fetchWorkbenchServicesis only dispatched whenservicesStatus === 'idle'. Sinceworkbenchstate is global and there's no reset wired to auth changes, switchingauth.userNameduring the SPA lifecycle can leaveworkbench.statusassucceededfor the previous user and prevent fetching services for the new user (and the fallback desktop endpoint may also contain the old username). Consider tracking the username the services were fetched for (e.g.,fetchedForUser) and refetch/reset when it differs from the currentauth.userName, or reset the workbench slice whensetUserNamechanges the user.
useEffect(() => {
if (servicesStatus === 'idle' && username) {
dispatch(
fetchWorkbenchServices({
url: `${appURL}/${username}/services`,
username,
}),
);
}
}, [servicesStatus, username, appURL, dispatch]);
|
All the non-preview links in Workbench Tools at test('Workbench Links open in new windows', async ({ page }) => {
await page.locator(`div[role="button"]:has-text("Workbench")`).click();
await expect(page).toHaveURL('./workbench');
await workbenchLinks.reduce(async (previousPromise, link) => {
await previousPromise;
const popupPromise = page.waitForEvent('popup');
await page.getByRole('link', { name: link.text }).click();
const popup = await popupPromise;
await popup.waitForLoadState('load', { timeout: 30000 });
const popupUrl = popup.url();
expect(popupUrl).toContain(link.url.replace('./', ''));
await popup.close();
return Promise.resolve();
}, Promise.resolve());
});The rest looks good. |
|
I found another potential problem in the feature/distributed-demo branch from Migrates React/Redux code out of preview (#1456). Making execution of twins fail and readme unavailable. Should I make a pull request to fix these Preview problems? |
|
@atomicgamedeveloper thanks for catching the bug. Can you please make a new PR addressing the two issues identified by you.
Thanks a lot. |



Type of Change
This pull request solves issue #1463.
Description
Replaces
mltooling/ml-workspace-minimal:0.13.2withintocps/workspace:latestacross all Docker Compose files. Removes hardcodedREACT_APP_WORKBENCHLINK_*env vars for the four workspace tools (VNC Desktop, VS Code, Jupyter Lab, Jupyter Notebook) — these links are now derived at runtime by fetching{appURL}/{username}/servicesfrom the new workspace image.New Redux slice —
workbench.slice.tsfetchWorkbenchServices(url): async thunk that GETs the services endpoint, validates the response with Zod, and stores resultssetWorkbenchServices/resetWorkbench: synchronous actions for seeding/resetting state in tests{ services: Record<string, WorkbenchService>, status: 'idle' | 'loading' | 'succeeded' | 'failed' }useWorkbenchLinkValues()—envUtil.tsMaps service keys (
desktop→VNCDESKTOP,vscode→VSCODE,lab→JUPYTERLAB,notebook→JUPYTERNOTEBOOK) from Redux toKeyLinkPair[]. Preview links (LIBRARY_PREVIEW,DT_PREVIEW) remain in env vars as they point to internal client routes.Workbench.tsxDispatches
fetchWorkbenchServiceson mount whenstatus === 'idle'and a username is present, ensuring a single fetch per session.Services JSON format (from
intocps/workspace):{ "desktop": { "name": "Desktop", "description": "...", "endpoint": "tools/vnc?path=..." }, "vscode": { "name": "VS Code", "description": "...", "endpoint": "tools/vscode" }, "lab": { "name": "Jupyter Lab", "description": "...", "endpoint": "lab" }, "notebook": { "name": "Jupyter Notebook", "description": "...", "endpoint": "" } }Config / type declaration cleanup
Removed
REACT_APP_WORKBENCHLINK_VNCDESKTOP/VSCODE/JUPYTERLAB/JUPYTERNOTEBOOKfrom all config files (dev.js,test.js,local.js,prod.js),env.d.ts, andconfigUtil.tspath keys. AddedAppDispatchexport tostore.ts.Testing
workbenchSlice.test.ts: reducer cases (pending/fulfilled/rejected, set, reset) plus thunk-level tests for valid JSON (fulfilled), invalid JSON rejected by Zod (failed), and non-ok HTTP response (failed)envUtil.test.ts: mocks Redux state with a services object; tests link construction from services and empty-services fallbackWorkbench.test.tsx(integration): pre-loads the store viasetWorkbenchServicesinstead of setting env varsConfig.test.tsxandglobal_mocks.tsxto reflect removed env varsCLI test fix (
cli/tests/test_utils.py)Three tests were asserting the old image name
mltooling/ml-workspace-minimal:0.13.2; updated tointocps/workspace:latestto match the already-updated YAML templates.Impact
Breaking config change: existing
env.jsdeployments must drop the fourREACT_APP_WORKBENCHLINK_*workspace entries. Workbench tool buttons will only render after the/servicesendpoint responds; a fetch failure sets status to'failed'with no workspace buttons shown (no crash).REACT_APP_WORKBENCHLINK_LIBRARY_PREVIEWandREACT_APP_WORKBENCHLINK_DT_PREVIEWare intentionally retained — they are internal client-side routes, not workspace endpoints.Additional Information
No new npm dependencies. Zod (already a project dependency) is used for response validation in the new slice.
Checklist