Yicong-Huang commented on code in PR #5151:
URL: https://github.com/apache/texera/pull/5151#discussion_r3293523975
##########
frontend/src/app/app-routing.module.ts:
##########
@@ -38,28 +38,21 @@ import { DatasetDetailComponent } from
"./dashboard/component/user/user-dataset/
import { UserDatasetComponent } from
"./dashboard/component/user/user-dataset/user-dataset.component";
import { HubWorkflowDetailComponent } from
"./hub/component/workflow/detail/hub-workflow-detail.component";
import { LandingPageComponent } from
"./hub/component/landing-page/landing-page.component";
-import { DASHBOARD_ABOUT, DASHBOARD_USER_WORKFLOW } from
"./app-routing.constant";
+import { USER_WORKFLOW } from "./app-routing.constant";
import { HubSearchResultComponent } from
"./hub/component/hub-search-result/hub-search-result.component";
import { AdminSettingsComponent } from
"./dashboard/component/admin/settings/admin-settings.component";
-import { GuiConfigService } from "./common/service/gui-config.service";
-
-const rootRedirectGuard: CanActivateFn = () => {
- const config = inject(GuiConfigService);
- const router = inject(Router);
- try {
- return router.parseUrl(DASHBOARD_ABOUT);
- } catch {
- // config not loaded yet, swallow the error and let the app handle it
- }
- return true;
-};
const routes: Routes = [];
routes.push({
- path: "dashboard",
+ path: "",
component: DashboardComponent,
Review Comment:
do we have plan to rename this component?
##########
frontend/src/app/app-routing.module.ts:
##########
@@ -174,18 +167,10 @@ routes.push({
],
});
-// default route renders the workspace editor directly; if userSystem is
enabled at runtime,
-// AppComponent will navigate to DASHBOARD_ABOUT instead.
-routes.push({
- path: "",
- component: WorkspaceComponent,
- canActivate: [rootRedirectGuard],
-});
-
Review Comment:
this seems unintended change. can we move it out from this PR?
##########
frontend/src/app/dashboard/component/user/list-item/list-item.component.spec.ts:
##########
@@ -104,4 +112,66 @@ describe("ListItemComponent", () => {
expect(component.entry.description).toBe("Old Description");
expect(component.editingDescription).toBe(false);
});
+
+ describe("initializeEntry routes", () => {
+ const baseStats = { likeCount: 0, viewCount: 0, isLiked: false };
+
+ it("routes owned workflows to the user workspace", () => {
+ component.currentUid = 1;
+ component.entry = {
+ id: 100,
+ type: "workflow",
+ workflow: { isOwner: true },
+ accessibleUserIds: [1],
+ ...baseStats,
+ } as unknown as DashboardEntry;
+ component.initializeEntry();
+ expect(component.entryLink).toEqual([USER_WORKSPACE, "100"]);
+ });
+
+ it("routes non-owned workflows to the hub workflow detail page", () => {
+ component.currentUid = 1;
+ component.entry = {
+ id: 101,
+ type: "workflow",
+ workflow: { isOwner: false },
+ accessibleUserIds: [2],
+ ...baseStats,
+ } as unknown as DashboardEntry;
+ component.initializeEntry();
+ expect(component.entryLink).toEqual([HUB_WORKFLOW_RESULT_DETAIL, "101"]);
+ });
+
+ it("routes projects to the user project page", () => {
+ component.entry = { id: 200, type: "project", ...baseStats } as unknown
as DashboardEntry;
+ component.initializeEntry();
+ expect(component.entryLink).toEqual([USER_PROJECT, "200"]);
+ });
Review Comment:
do we still route projects?
cc @mengw15 @carloea2
##########
frontend/src/app/dashboard/component/dashboard.component.ts:
##########
@@ -249,6 +249,6 @@ export class DashboardComponent implements OnInit {
}
}
- protected readonly DASHBOARD_ABOUT = DASHBOARD_ABOUT;
+ protected readonly ABOUT = ABOUT;
protected readonly String = String;
}
Review Comment:
shall we take the chance to move those two up with other constant
definitions?
##########
frontend/src/app/dashboard/component/dashboard.component.spec.ts:
##########
@@ -162,4 +177,48 @@ describe("DashboardComponent", () => {
expect(fixture.debugElement.query(By.css("#powered-by"))).toBeTruthy();
});
+
+ it("should hide the navbar on workflow workspace routes", () => {
+ expect(component.isNavbarEnabled("/user/workflow/42")).toBe(false);
+ expect(component.isNavbarEnabled("/user/workflow")).toBe(true);
+ expect(component.isNavbarEnabled("/user/project")).toBe(true);
+ });
+
+ it("exposes route constants without the legacy /dashboard prefix", () => {
+ expect(USER_PROJECT).toBe("/user/project");
Review Comment:
ditto. I think project is deprecated, shall we disable routing towards it?
cc @mengw15 @carloea2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
