Review/coordinator by ritwikshanker · Pull Request #92 · EOSC-Data-Commons/matchmaker

ritwikshanker · 2026-04-22T08:55:17Z

No description provided.

All mocking moved to coordinator. Tool service and dispatcher service works together there.

…atching

Copilot

Pull request overview

This PR introduces a new “Dataplayer” flow backed by a coordinator service (gRPC on the UI server + HTTP/SSE wrappers for the browser), updates the UI to route users to the new dataplayer page, and keeps a legacy “dispatcher” run path under a new deprecated route.

Changes:

Add coordinator-backed dataplayer UI (DataplayerPage) and client wrappers (coordinatorApi.ts) plus server-side endpoints in server.ts.
Add a Node gRPC client and generated protobuf TS artifacts for coordinator integration.
Replace/remove legacy dispatcher route/API pieces and add a deprecated legacy run page.

Reviewed changes

Copilot reviewed 19 out of 23 changed files in this pull request and generated 22 comments.

Show a summary per file

File	Description
src/types/dispatcher.ts	Removes dispatcher-related shared types (currently still referenced by legacy code).
src/types/dataplayerTypes.ts	Adds dataplayer/coordinator request & response types.
src/routes.ts	Replaces dispatcher route with dataplayer + legacy dataplayer routes.
src/pages/LandingPage.tsx	Adds a new “play” input that opens the dataplayer page.
src/pages/DataplayerPage.tsx	New dataplayer UI: file listing, tool matching/search, slot mapping, SSE monitoring.
src/pages/DataplayerLegacyRunPage.tsx	Keeps legacy run path but rewired to coordinator “*-old” endpoints.
src/lib/utils.ts	Changes dev/prod logging behavior (currently forced to dev).
src/lib/server/grpcClient.ts	Adds gRPC client logic for datasets/tools/launching.
src/lib/server/generated/google/protobuf/timestamp.ts	Generated protobuf runtime types.
src/lib/server/generated/google/protobuf/struct.ts	Generated protobuf runtime types.
src/lib/dispatcherApi.ts	Removes old dispatcher API client.
src/lib/deprecatedDispatcherApi.ts	Reintroduces dispatcher submission/status logic as “deprecated”.
src/lib/coordinatorApi.ts	Adds browser-facing API wrapper for coordinator endpoints.
src/components/SearchResultItem.tsx	Updates CTA buttons to “Play” (new dataplayer) + legacy run button.
src/components/DataplayInput.tsx	Adds new landing-page input component for dataplayer.
server.ts	Adds coordinator HTTP endpoints + SSE bridging; adds proxy route.
package.json / package-lock.json	Adds grpc/jsonwebtoken/tooling deps.
eslint.config.js	Updates ESLint config (currently has an overriding languageOptions bug).
README.md	Documents coordinator/gRPC codegen + submodule workflow.
.gitmodules	Adds `req-packager` submodule.

Comments suppressed due to low confidence (3)

package.json:44

Generated gRPC code imports @bufbuild/protobuf/wire at runtime (via src/lib/server/generated/...), but @bufbuild/protobuf is not listed in dependencies. If production installs omit devDependencies, the server will crash on startup. Add @bufbuild/protobuf to dependencies (not just via a devDependency transitive).

  "dependencies": {
    "@grpc/grpc-js": "^1.14.3",
    "@radix-ui/react-toast": "^1.2.15",
    "@radix-ui/react-tooltip": "^1.2.8",
    "@react-router/express": "^7.13.0",
    "@react-router/fs-routes": "^7.13.0",
    "@react-router/node": "^7.13.0",
    "@react-router/serve": "^7.13.0",
    "@tailwindcss/vite": "^4.1.17",
    "@tanstack/react-query": "^5.90.21",
    "class-variance-authority": "^0.7.1",
    "compression": "^1.8.1",
    "express": "^5.2.1",
    "http-proxy-middleware": "^3.0.5",
    "isbot": "^5.1.35",
    "jsonwebtoken": "^9.0.3",
    "lucide-react": "^0.563.0",
    "morgan": "^1.10.1",
    "pm2": "^6.0.14",
    "react": "^19.2.4",
    "react-dom": "^19.2.4",
    "react-icons": "^5.5.0",
    "react-router": "^7.13.0",
    "tailwind-merge": "^3.4.0",
    "tailwindcss-animate": "^1.0.7"
  },

eslint.config.js:25

This ESLint config sets languageOptions twice in the same config object. The second languageOptions (line 21+) overrides the first one, so the configured parser and parserOptions are effectively dropped. Merge these into a single languageOptions block so parser + parserOptions + globals are all applied together.

  {
    languageOptions: {
      parser: '@typescript-eslint/parser',
      parserOptions: {
        ecmaVersion: 2020,
        sourceType: 'module',
        ecmaFeatures: { jsx: true },
      },
      globals: globals.browser,
    },
    extends: [js.configs.recommended, ...tseslint.configs.recommended],
    files: ['**/*.{ts,tsx}'],
    languageOptions: {
      ecmaVersion: 2020,
      globals: globals.browser,
    },
    plugins: {

src/pages/DataplayerLegacyRunPage.tsx:18

DataplayerLegacyRunPage still imports from ../types/dispatcher, but src/types/dispatcher.ts was removed. This will cause module resolution failures. Either restore the dispatcher types module or update this page (and related utils) to import the types from their new location.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-22T09:06:59Z

+    const datasetTitle = searchParams.get('title');
+    const datasetHandle = searchParams.get('datasetId');
+    const navigate = useNavigate();
+
+    // Step management
+    const [currentStep, setCurrentStep] = useState<StepType>('select-analysis');
+    // tool uuid
+    const [selectedToolId, setSelectedToolId] = useState<string>(null);
+
+    // File management
+    const [files, setFiles] = useState<FileMeta[]>([]);
+    const [fileParameterMappings, setFileParameterMappings] = useState<Record<number, string>>({});
+    const [loadingFiles, setLoadingFiles] = useState(false);
+    const [filesError, setFilesError] = useState<string | null>(null);
+
+    const [loading, setLoading] = useState(true);
+    const [error, setError] = useState<string | null>(null);
+
+    useEffect(() => {
+        const load = async () => {
+            console.log("Start loading");
+            try {
+                setLoading(true);
+                const files = await fetchFilesMetaByDatasetHandle(datasetHandle);
+                setFiles(files);
+            } catch (err) {
+                console.error(err);
+                setError("Failed to fetch files");
+            } finally {
+                setLoading(false);
+                console.log("Finished loading");
+            }
+        };
+
+        load();
+    }, [datasetHandle]);


datasetHandle is read from the URL and can be null, but it’s passed directly into fetchFilesMetaByDatasetHandle(). This can result in requests like handle=null and confusing UI states. Add a guard that shows a clear error (or redirects) when datasetId is missing/invalid.

Copilot · 2026-04-22T09:06:59Z

                    <a href={hit._id} target="_blank" rel="noopener noreferrer"
-                       aria-label={`View dataset ${hit.title}`}
-                       className="inline-flex items-center justify-center gap-1 rounded-md bg-green-600 px-3 py-1.5 text-sm font-medium text-white shadow-sm hover:bg-green-700 focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-green-600 transition-colors">
+                        aria-label={`Redinect to the source of dataset ${hit.title}`}


Typo in aria-label: “Redinect” → “Redirect”.

Suggested change

aria-label={`Redinect to the source of dataset ${hit.title}`}

aria-label={`Redirect to the source of dataset ${hit.title}`}

Copilot · 2026-04-22T09:07:00Z

+        client.getArtifact(grpc_req, (err, response) => {
+            if (err) {
+                console.error(err);
+                return;
+            }
+            let callbackUrl: string | undefined;
+
+            if (response.eoscInline) {
+                callbackUrl = response.eoscInline.callbackUrl;
+            } else if (response.hosted) {
+                callbackUrl = response.hosted.callbackUrl;
+            } else {
+                console.error("No entry_point found");
+                return;
+            }
+            v_callbackUrl = callbackUrl;
+            res.send(v_callbackUrl);
+        });


In /api/coordinator/tasks-result/:taskId, if getArtifact returns an error or no entry point, the handler logs and returns without sending a response. This leaves the HTTP request hanging. Respond with an appropriate status (e.g., 502/500) in the error branch and when no entry point is found.

Copilot · 2026-04-22T09:07:00Z

+process.on("SIGINT", () => {
+    console.log("SIGINT received: closing gRPC client...");
+    // closes HTTP/2 channel to avoid resource leaking
+    db_client.close(); 
+    process.exit();
+});
+
+process.on("SIGTERM", () => {
+    console.log("SIGTERM received: closing gRPC client...");
+    db_client.close();


db_client can be null when SIGINT/SIGTERM handlers run (e.g., no RPCs made yet), but the code calls db_client.close() unconditionally. This will throw during shutdown. Use a null-check/optional chaining and consider also closing the other singleton clients created in this module.

Suggested change

process.on("SIGINT", () => {

console.log("SIGINT received: closing gRPC client...");

// closes HTTP/2 channel to avoid resource leaking

db_client.close();

process.exit();

});

process.on("SIGTERM", () => {

console.log("SIGTERM received: closing gRPC client...");

db_client.close();

function closeGrpcClients(): void {

// closes HTTP/2 channel to avoid resource leaking

db_client?.close();

db_client = null;

}

process.on("SIGINT", () => {

console.log("SIGINT received: closing gRPC client...");

closeGrpcClients();

process.exit();

});

process.on("SIGTERM", () => {

console.log("SIGTERM received: closing gRPC client...");

closeGrpcClients();

Copilot · 2026-04-22T09:07:00Z

+    // XXX: if I deploy the grpc server with client in the same NAT, I can use insecure channel, but if goes to ethz deployment, should not.
+    // Should use SSL for msg over wire.
+    const client = new DataplayerServiceClient(GRPC_TARGET, createInsecureChannel());
+
+    const msgSlotsMapping = {} as {string: FileEntry};
+    for (const k in slotsMapping) {
+        msgSlotsMapping[k] = fileMetaToFileEntry(slotsMapping[k]); 
+    }
+
+    const request: LaunchToolRequest = {
+        toolId,
+        slotsMapping: msgSlotsMapping,
+    }
+
+    return new Promise((resolve, reject) => {
+        client.launchTool(request, metadata, (error, response) => {
+            if (error) {
+                // TODO: log needed?
+                reject(error);
+                return;
+            }
+
+            resolve(response.handlerId);
+        }); 
+    });
+}


msgSlotsMapping is typed as {} as {string: FileEntry}, which isn’t a valid index signature type and defeats type-checking. Use a proper map type (e.g. Record<string, FileEntry>) and consider closing/reusing the gRPC client after launchTool completes to avoid leaking channels.

Copilot · 2026-04-22T09:07:04Z

+            // XXX: should I close the client??
+            client.close();
+            resolve(files);
+        });
+
+        stream.on("error", (err: grpc.ServiceError) => {
+            client.close();


fetchDatasetFilesFromDatadaseByUUID (typo in name) calls client.close() on end/error, but client comes from getDatasetClient() which returns a singleton. Closing it here can break subsequent calls that reuse the singleton. Either don’t close singleton clients per call, or stop using a singleton for this RPC.

Suggested change

// XXX: should I close the client??

client.close();

resolve(files);

});

stream.on("error", (err: grpc.ServiceError) => {

client.close();

// Do not close the dataset client here; getDatasetClient() returns

// a shared singleton that must remain available for subsequent RPCs.

resolve(files);

});

stream.on("error", (err: grpc.ServiceError) => {

Copilot · 2026-04-22T09:07:04Z

+import {
+    DispatcherResult,
+    FileMetrixFile,
+    TaskStatus,
+    TaskStatusResponse,
+} from "../types/dispatcher";
+import { DISPATCHER_CONFIGS, updateOnedataForTarget } from "./dispatcherUtils";
+import { fetchWithTimeout } from "./utils";
+


This file imports dispatcher types from ../types/dispatcher, but src/types/dispatcher.ts was removed in this PR. That will break builds for the legacy dispatcher path. Either keep the dispatcher type definitions (e.g., reintroduce src/types/dispatcher.ts) or move these legacy types into a new module and update imports accordingly.

Suggested change

import {

DispatcherResult,

FileMetrixFile,

TaskStatus,

TaskStatusResponse,

} from "../types/dispatcher";

import { DISPATCHER_CONFIGS, updateOnedataForTarget } from "./dispatcherUtils";

import { fetchWithTimeout } from "./utils";

import { DISPATCHER_CONFIGS, updateOnedataForTarget } from "./dispatcherUtils";

import { fetchWithTimeout } from "./utils";

type TaskStatus = string;

interface DispatcherResult {

task_id: string;

[key: string]: unknown;

}

interface FileMetrixFile {

ro_crate_extensions: unknown;

[key: string]: unknown;

}

interface TaskStatusResponse {

status: TaskStatus;

task_id?: string;

[key: string]: unknown;

}

Copilot · 2026-04-22T09:07:04Z

+            const tools = await searchToolsByText(toolSearchText);
+            setQueryToolResults(tools);
+        }
+
+        load();
+    }, [debouncedSearch, toolSearchText]);


The debounced search effect triggers on debouncedSearch, but the API call uses toolSearchText instead. This defeats the debounce and can issue requests with a newer/older value than the effect intended. Use debouncedSearch as the query inside the effect and remove the extra dependency on toolSearchText.

Suggested change

const tools = await searchToolsByText(toolSearchText);

setQueryToolResults(tools);

}

load();

}, [debouncedSearch, toolSearchText]);

const tools = await searchToolsByText(debouncedSearch);

setQueryToolResults(tools);

}

load();

}, [debouncedSearch]);

Copilot · 2026-04-22T09:07:05Z

+                                                </td>
+                                                <td className="px-6 py-4">
+                                                    <select
+                                                        onChange={(e) => handleSlotSet(Number(e.target.value), param)}
+                                                        className="block w-full px-3 py-2 text-sm border border-gray-300 rounded-md shadow-sm focus:outline-none focus:ring-blue-500 focus:border-blue-500"
+                                                    >
+                                                        <option key="none" value="None">--select to set parameter--</option>
+                                                        {
+                                                            files.map((file, fileIndex) => (
+                                                                <option key={fileIndex} value={fileIndex}>
+                                                                    {file.filename}
+                                                                </option>
+                                                            ))
+                                                        }
+                                                    </select>


In the slot→file mapping table, the “none” option has value="None" but onChange does Number(e.target.value), so selecting it will pass NaN as the file index and corrupt mappings. Also, because the slot name is fixed (param), there’s currently no way to unassign a slot once set. Use a numeric sentinel for “none” (or a separate handler) and update handleSlotSet/mapping model so slots can be cleared safely.

Copilot · 2026-04-22T09:07:05Z

+app.get("/api/coordinator/task-status/:taskId", async (req, res) => {
+    // TODO: token or session cookie to prevent access from anywhere.
+
+    const { taskId } = req.params;
+
+    // SSE headers
+    res.setHeader("Content-Type", "text/event-stream");
+    res.setHeader("Cache-Control", "no-cache");
+    res.setHeader("Connection", "keep-alive");
+
+    // XXX: per user connection as well, so the token or the user name need to be an argument passed in
+    const client = getDataplayerClient();
+    const grpc_req: MonitorStateRequest = {
+        id: taskId,
+    };
+    const stream = client.monitorState(grpc_req);
+    let lastState: ToolState_State | null = null;
+
+    stream.on("data", (resp: MonitorStateResponse) => {
+    // when state machine transit to the end state.
+        const toolState = resp.status;
+        const currentState = toolState?.state ?? null;
+
+        // Stream progress
+        if (currentState && currentState != lastState) {
+            lastState = currentState;
+
+            let stateStr = null;
+            // XXX: ??? why currentState don't match?
+            // if (currentState === ToolState_State.PREPARING) {
+            //     stateStr = "PREPARING";
+            // }
+            if (currentState === ToolState_State.READY) {
+                stateStr = "READY";
+            }
+            if (currentState === ToolState_State.DROPPED) {
+                stateStr = "DROPPED";
+            }
+            if (currentState === ToolState_State.UNRECOGNIZED) {
+                stateStr = "UNRECOGNIZED";
+            }
+            res.write(`event: state\ndata: ${
+                JSON.stringify({
+                    state: stateStr,
+                    message: toolState?.log,
+                })
+            }\n\n`);
        }
+    });
+
+    stream.on("end", () => {
+        res.end();
+    });
+});


The SSE endpoint for task status doesn’t handle client disconnects or gRPC stream errors. Without req.on('close', ...) to cancel/close the gRPC stream (and an stream.on('error', ...) handler), the server can leak listeners/streams if the browser navigates away or the RPC fails.

All mocking moved to coordinator. Tool service and dispatcher service works together there.

…atching

…coordinator # Conflicts: # src/lib/dispatcherApi.ts

unkcpz added 25 commits March 26, 2026 08:45

grpc client codegen from protobuf

ae93fc6

Coordinator/dataplayer impl

d2e299b

file and folder list in view page

d1dc974

Remove dispatcher run page impl

5a29fd5

Move all grpc client call to server.ts

ee03232

grpc clients as singletons for performance

c36bcec

eslint config for auto formatting

34ede2a

sse like task update

728c177

keep the old run page and use grpc client to re-waven one

99fafd0

filemapping to the selected tool

099fc90

dataplayer launch tool feat

66bf6cb

Add req-packager as submodule for protobuf sync

b26854d

refact and readme on comm data flow

ccf9eaf

submodule: coordinator add health check

f8cf013

remove proto/service.proto

0fa6268

dettach the mocking of tool from matchmaker

b8de360

All mocking moved to coordinator. Tool service and dispatcher service works together there.

combine file view and vre view

c177f69

column layout of file panel and vre panal

47f3c17

coordinator improvement from/into conversion

d8121fd

Update the wording

a233791

real tool registry service in use

937212f

Search tool by text

a51e4f8

match tools by files

580b9e1

Type shared in the boundary of json payload transfering to avoid mism…

774c981

…atching

add direct play without search

f487cb1

Copilot AI review requested due to automatic review settings April 22, 2026 08:55

Copilot started reviewing on behalf of ritwikshanker April 22, 2026 08:55 View session

Copilot AI reviewed Apr 22, 2026

View reviewed changes

unkcpz added 2 commits April 22, 2026 11:24

grpc client codegen from protobuf

cb634b2

Coordinator/dataplayer impl

bc81d46

unkcpz and others added 24 commits April 22, 2026 11:24

file and folder list in view page

fe389ef

Remove dispatcher run page impl

b719e37

Move all grpc client call to server.ts

b3f76e2

grpc clients as singletons for performance

cc54b7a

eslint config for auto formatting

6bc2518

sse like task update

70a9bc9

keep the old run page and use grpc client to re-waven one

b7f94ca

filemapping to the selected tool

3ce6cfb

dataplayer launch tool feat

54b9aad

Add req-packager as submodule for protobuf sync

4bd8788

refact and readme on comm data flow

32b8404

submodule: coordinator add health check

821fc4f

remove proto/service.proto

a34e322

dettach the mocking of tool from matchmaker

89f0616

All mocking moved to coordinator. Tool service and dispatcher service works together there.

combine file view and vre view

7559bd3

column layout of file panel and vre panal

9f72fc5

coordinator improvement from/into conversion

1e007eb

Update the wording

b334976

real tool registry service in use

125b197

Search tool by text

9bfcff2

match tools by files

46ddf58

Type shared in the boundary of json payload transfering to avoid mism…

b8ac230

…atching

add direct play without search

857276d

Merge remote-tracking branch 'origin/review/coordinator' into review/…

4f2f776

…coordinator # Conflicts: # src/lib/dispatcherApi.ts

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review/coordinator#92

Review/coordinator#92
ritwikshanker wants to merge 51 commits intorelease/0.7.xfrom
review/coordinator

ritwikshanker commented Apr 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 22, 2026

Uh oh!

Copilot AI Apr 22, 2026

Uh oh!

Copilot AI Apr 22, 2026

Uh oh!

Copilot AI Apr 22, 2026

Uh oh!

Copilot AI Apr 22, 2026

Uh oh!

Copilot AI Apr 22, 2026

Uh oh!

Copilot AI Apr 22, 2026

Uh oh!

Copilot AI Apr 22, 2026

Uh oh!

Copilot AI Apr 22, 2026

Uh oh!

Copilot AI Apr 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	aria-label={`Redinect to the source of dataset ${hit.title}`}
	aria-label={`Redirect to the source of dataset ${hit.title}`}

-import {
-    DispatcherResult,
-    FileMetrixFile,
-    TaskStatus,
-    TaskStatusResponse,
-} from "../types/dispatcher";
-import { DISPATCHER_CONFIGS, updateOnedataForTarget } from "./dispatcherUtils";
-import { fetchWithTimeout } from "./utils";
+import { DISPATCHER_CONFIGS, updateOnedataForTarget } from "./dispatcherUtils";
+import { fetchWithTimeout } from "./utils";
+type TaskStatus = string;
+interface DispatcherResult {
+    task_id: string;
+    [key: string]: unknown;
+}
+interface FileMetrixFile {
+    ro_crate_extensions: unknown;
+    [key: string]: unknown;
+}
+interface TaskStatusResponse {
+    status: TaskStatus;
+    task_id?: string;
+    [key: string]: unknown;
+}

Conversation

ritwikshanker commented Apr 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants