refactor: update shell layout - #89
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Comment |
vasylcf
left a comment
vasylcf
left a comment
There was a problem hiding this comment.
Reviewed the shell-levels refactor against the shipped SDLC docs (shell-levels.md, workspaces-screen.md, organization-overview.md). The overall design is solid and well documented, but there's one real feature regression that contradicts this PR's own acceptance criteria, plus several smaller correctness/UX issues worth fixing before merge.
| case 'settings': | ||
| return <SettingsSection project={project} config={config} />; | ||
| default: | ||
| return <PlaceholderSection title={t(`section_${section}`)} note={t('no_source_yet')} />; |
There was a problem hiding this comment.
Blocking. The switch (section) here lost its 'team' and 'settings' cases, and SettingsSection.tsx was deleted outright (0 bytes in this diff). Both sections now fall through to default → PlaceholderSection ("no source yet").
mfe.json still registers "Team" (section: 'team') and "Project settings" (section: 'settings', order 100) as live rail items for the project level, and this PR's own acceptance criteria in shell-levels.md requires "Opening a project shows the project's rail — Overview, Artifacts, Findings, Activity, Timeline, Team, and settings last". Right now clicking either one silently shows a placeholder instead of real content — a functional regression, not a refactor.
Either restore the two sections or explicitly drop those rail items (and update the AC) if removing them is intentional.
There was a problem hiding this comment.
Artifacts is the only section with a content. The rest - settings and team among them - are declared in the manifest so the rail is whole, and show the placeholder until there is something to read there.
| const screens = registry.getExtensionsForDomain(screenDomain.id) as ScreenExtension[]; | ||
| const target = entryPointOf(screens, level); | ||
| if (!target) return; | ||
| mountScreen(registry, target) |
There was a problem hiding this comment.
Should-fix. This app/context/level/requested handler calls mountScreen() with no re-entrancy guard. Rail.tsx's choose() guards the identical mountScreen() call with a mounting state flag specifically to prevent a double-mount race, but this handler — the one ContextChain's breadcrumb slots call via enter() — has no equivalent guard. Two quick clicks on different breadcrumb slots (or on the same one before the first mount resolves) can fire this handler twice concurrently and race two mounts against each other.
| )} | ||
| <WorkspacesTable | ||
| rows={rows} | ||
| total={rows.length} |
There was a problem hiding this comment.
Should-fix. WorkspacesTable is given total={rows.length} — the already search-filtered array — instead of all.length (the sibling WorkspacesToolbar on line 80 correctly uses all.length for the same screen). WorkspacesTable's footer renders {from}-{to} of {total}, so as soon as a search narrows the list, the caption becomes tautological (e.g. "1-2 of 2" instead of reflecting the organization's real workspace count).
| @@ -41,30 +30,20 @@ export function MfeScreenContainer() { | |||
| eventBus.emit('app/mfe/bootstrap', { status: 'failed' }); | |||
There was a problem hiding this comment.
Should-fix. On a bootstrap failure, bootstrapped is never set to true, so this container renders nothing (as before). What changed is the surrounding navigation: the deleted Menu.tsx used to show "Screens could not be loaded. Check the console for the manifest error." in the nav area on this exact status. Rail.tsx (its replacement) just returns null when items.length <= 1, which is also true when bootstrap fails with no extensions registered. Net effect: a failed/unreachable manifest now leaves the whole shell blank with no user-facing indication anything went wrong, where before there was an explicit message.
| const t = useWorkspacesText(); | ||
| const [query, setQuery] = useState(""); | ||
| const { data, isLoading, isError, refetch } = useApiQuery( | ||
| accounts.getWorkspaces({ organizationId: organization.id }), |
There was a problem hiding this comment.
Should-fix. This fetches the org's workspace list via accounts.getWorkspaces(...) with staleTime: 0, which duplicates the same tenant-children-of-type read the shell already performs and caches in appContextSlice (resolveWorkspaces in appContextEffects.ts). The doc for this feature accepts "two readers of one AM endpoint" as a tradeoff for keeping this screen decoupled from the shell's channel, which is a reasonable call — but staleTime: 0 means every time this screen mounts (or the scoped workspace changes, per the effect below) it re-fetches from the network rather than reusing react-query's cache, so it's paying that cost more often than necessary.
| const [open, setOpen] = useState(false); | ||
| const [mounting, setMounting] = useState(false); | ||
|
|
||
| const closeOnBlur = useCallback((event: React.FocusEvent<HTMLDivElement>) => { |
There was a problem hiding this comment.
Should-fix. Rail only closes via onPointerLeave/onBlur (focus or pointer leaving the subtree) — there's no onKeyDown handling anywhere in app/layout for this component. A keyboard user who tabs into the Rail and opens it has no way to collapse it again short of tabbing all the way out of the subtree. Worth adding an Escape handler here, consistent with the removed Menu.tsx's behavior.
| announceSection(bridge, 'artifacts'); | ||
| start(); | ||
| }, [isFirstImport, dispatch, start]); | ||
| }, [isFirstImport, dispatch, start, bridge]); |
There was a problem hiding this comment.
Cleanup. bridge (from useMfeBridge()) is now in this effect's dependency array alongside start. If bridge isn't referentially stable across renders, a re-render before isFirstImport flips to false (the dispatch is async relative to this render) could re-run the effect and call start()/announceSection() a second time for the same first-import. Worth double-checking useMfeBridge()'s memoization, or guarding this effect with a ref the way MfeScreenContainer's bootstrap does.
| return; | ||
| } | ||
| void refetch(); | ||
| }, [scopedWorkspace, refetch]); |
There was a problem hiding this comment.
Cleanup. This effect only special-cases the very first invocation via seenScoped.current; every subsequent run — triggered either by scopedWorkspace changing or by refetch's identity changing — calls refetch() unconditionally. If useApiQuery's refetch isn't stable per query instance, a refetch-triggered re-render could produce a new refetch reference and re-fire this effect, looping redundant network calls. Worth confirming refetch's stability or dropping it from the dependency array.
| */ | ||
| export function publishStudioContext(app: FrontXApp): void { | ||
| publishSelectedOrganization(app); | ||
| publishSelectedWorkspace(app); |
There was a problem hiding this comment.
Cleanup. publishStudioContext (called once at bootstrap) publishes org/workspace/project/session but not publishSelectedSection, even though project_section.selected is a required property declared by organization-mfe and projects-mfe. Every other context property gets seeded to null at bootstrap; this one stays genuinely undefined until the first mountScreen() call emits it asynchronously. Today's section === 'settings' string checks happen to fall through safely on undefined, but it's an inconsistent guarantee versus the other properties — worth adding the missing call for consistency.
| import type { ChildMfeBridge } from '@gears-frontx/react'; | ||
|
|
||
| /** Screen domain: `kind: opened | closed | section`. */ | ||
| export const STUDIO_ACTION_CONTEXT_PUBLISH = |
There was a problem hiding this comment.
Cleanup. STUDIO_ACTION_CONTEXT_PUBLISH and STUDIO_ACTION_WORKSPACES_PUBLISH are hand-typed as identical string literals here and independently in app/mfe/contextActions.ts, rather than one importing from the other. A future rename or copy/paste typo in either file would silently desync host and child MFEs, surfacing only at runtime as a rejected or no-op action chain with no build-time error.
Duplicated AccountsApiService implementation left in shell after extraction to shared packageSeverity: Minor Problem Reproduction, impact, suggested fix, verificationHow to reproduce
Expected behavior Actual behavior Impact Suggested correction How to verify Original location: studio-frontend/src-app/app/api/AccountsApiService.ts:1 -- inline anchoring could not be resolved after 1 attempt(s). |
Signed-off-by: Maryna Lituyeva <maryna.lituyeva@constructor.tech>
Signed-off-by: Maryna Lituyeva <maryna.lituyeva@constructor.tech>
Signed-off-by: Maryna Lituyeva <maryna.lituyeva@constructor.tech>
Signed-off-by: Maryna Lituyeva <maryna.lituyeva@constructor.tech>
4f5477e to
50f2380
Compare
Signed-off-by: Maryna Lituyeva <maryna.lituyeva@constructor.tech>
f34d9fb to
02987df
Compare
Fixed |
| workspaceId: payload.workspace.id, | ||
| name: payload.workspace.name, | ||
| ...scopeOf(payload, 'organizationId'), | ||
| }); |
There was a problem hiding this comment.
Stale cross-organization workspace selection still forces a level change even when the workspace data itself is correctly rejected
Severity: Major
Problem
createWorkspacePublishHandler's 'selected' branch in contextActions.ts emits two independent events for one user action: 'app/context/workspace/changed' (carrying organizationId, now correctly guarded by staleScope in appContextEffects.ts) and an unconditional 'app/context/level/requested' ({level:'workspace'}) with no organization/scope information at all. The level/requested listener in appContextEffects.ts resolves the workspace level's entry screen and calls enterScreen/mountScreen purely based on 'level', independent of whether the paired workspace/changed announcement was accepted or dropped as stale.
Reproduction, impact, suggested fix, verification
How to reproduce
- User is in Organization A and clicks a workspace row in organization-mfe's WorkspacesScreen, triggering requestWorkspace with organizationId=A (async, in flight).
- Before the action resolves, the user switches the shell's context to Organization B.
- The delayed 'selected' action from step 1 arrives, contextActions.ts emits workspace/changed with organizationId=A (dropped by staleScope since current org is now B) and level/requested {level:'workspace'} (no scope check).
- appContextEffects.ts's level/requested handler mounts the workspace-level entry screen regardless.
Expected behavior
A stale/late workspace selection should be fully ignored -- neither the stored workspace context nor the mounted screen level should change when the announcement's organization no longer matches the current one.
Actual behavior
The workspace data is dropped, but the shell still navigates into the workspace-level screen (e.g. mounts Projects/workspace entry), producing a screen level/breadcrumb inconsistent with the actual (unchanged) organization/workspace context.
selected action (orgId=A) --> emit workspace/changed(orgId=A) --[staleScope: dropped, org now B]--> (no state change)
\--> emit level/requested(level=workspace) --[no scope check]--> enterScreen/mountScreen(workspace entry) --> UI now shows workspace-level screen with no valid workspace context
Impact
Users can be navigated into a workspace-level screen/UI with no valid workspace selected for the current organization, producing inconsistent breadcrumbs, an orphaned rail/level state, and a confusing screen mount that doesn't match stored context.
Suggested correction
Either pass organizationId through the level/requested event and have its handler apply the same staleScope check before mounting, or only emit level/requested from the workspace/changed handler itself (after it has already validated/applied the scope) rather than unconditionally from the action handler.
How to verify
Add a test simulating a delayed 'selected' workspace action whose organizationId differs from the currently active organization at delivery time, and assert that no screen mount/level change occurs and the previously mounted screen/level is retained.
There was a problem hiding this comment.
Re-verified against the current code -- this write-up has been updated.
Why
Problem (was): createWorkspacePublishHandler's 'selected' branch emits both 'app/context/workspace/changed' (guarded by staleScope in appContextEffects.ts) and 'app/context/level/requested' unconditionally and independently. Only the first event's handler checks staleScope(currentOrgId(app), organizationId); the second event's handler (eventBus.on('app/context/level/requested',...)) has no scope check and unconditionally resolves and mounts the workspace-level entry screen via enterScreen/mountScreen.
Problem (now): createWorkspacePublishHandler's 'selected' branch in contextActions.ts emits two independent events for one user action: 'app/context/workspace/changed' (carrying organizationId, now correctly guarded by staleScope in appContextEffects.ts) and an unconditional 'app/context/level/requested' ({level:'workspace'}) with no organization/scope information at all. The level/requested listener in appContextEffects.ts resolves the workspace level's entry screen and calls enterScreen/mountScreen purely based on 'level', independent of whether the paired workspace/changed announcement was accepted or dropped as stale.
| eventBus.emit('mfe/projects/open-requested', project); | ||
| publish(bridge, { kind: 'opened', project, siblings }); | ||
| publish(bridge, { kind: 'opened', project, siblings, ...(workspaceId ? { workspaceId } : {}) }); | ||
| } |
There was a problem hiding this comment.
Null workspaceId is dropped from project announcement payload, bypassing staleScope rejection
Severity: Major
Problem
requestOpenProject and announceCreatedProject use ...(workspaceId ? { workspaceId }: {}), so a null or empty-string workspaceId (explicitly typed and passed by callers like ProjectsTable's workspace?.id ?? null) is omitted from the wire payload rather than sent as an explicit claim. scopeOf() on the shell then produces {} for that key, and staleScope(current, undefined) is defined to return false (never stale) for an omitted claim.
Reproduction, impact, suggested fix, verification
How to reproduce
- ProjectsTable renders before
workspaceis resolved (workspace undefined -> workspace?.id ?? null). 2. User clicks a project row; requestOpenProject is called with workspaceId=null. 3. Payload sent to shell omits workspaceId entirely. 4. Meanwhile the shell's active workspace is some other workspace W (e.g. user already switched). 5. appContextEffects' 'app/context/project/opened' and 'app/context/projects' listeners compute staleScope(W, undefined) = false, so the announcement's project/siblings are applied to W anyway.
Expected behavior
An unresolved/null workspaceId claim should still be distinguishable from a genuinely unscoped announcement, or the shell should independently validate that the announced project actually belongs to the currently selected workspace before applying it.
Actual behavior
The null workspaceId is silently coalesced away by the spread, collapsing into the same 'unclaimed' bucket that staleScope treats as always-current.
workspace?.id ?? null -> requestOpenProject(workspaceId=null) -> spread omits key -> scopeOf() = {} -> staleScope(current, undefined) = false -> project/siblings applied to wrong workspace
Impact
A delayed or unresolved project selection/creation can populate the active workspace's breadcrumb and project switcher with another workspace's project data.
Suggested correction
Send workspaceId explicitly (including null) rather than conditionally spreading it, and change staleScope/scopeOf to treat an explicit null claim as always mismatching the current (non-null) workspace, rather than as unclaimed.
How to verify
Unit test: call requestOpenProject with workspaceId=null while the shell has a different non-null active workspace, and assert the announcement is rejected/ignored rather than applied.
There was a problem hiding this comment.
Re-verified against the current code -- this write-up has been updated.
Why
Problem (was): requestOpenProject/announceCreatedProject in projects-mfe's projectsActions.ts accept workspaceId: string | null but build the outgoing payload with ...(workspaceId ? { workspaceId }: {}), so a null (or empty-string) workspaceId is omitted entirely rather than sent as an explicit scope claim. On the shell side, contextActions.ts's scopeOf() only sets {workspaceId} when the payload key is a string, and appContextEffects.ts's staleScope() explicitly treats an undefined claimed scope as never stale ('claimed !== undefined && claimed !== current'). An omitted key is therefore indistinguishable from a sender that never named a scope, and the sibling project list / opened-project announcement is applied to whatever workspace the shell currently has selected.
Problem (now): requestOpenProject and announceCreatedProject use ...(workspaceId ? { workspaceId }: {}), so a null or empty-string workspaceId (explicitly typed and passed by callers like ProjectsTable's workspace?.id ?? null) is omitted from the wire payload rather than sent as an explicit claim. scopeOf() on the shell then produces {} for that key, and staleScope(current, undefined) is defined to return false (never stale) for an omitted claim.
|
|
||
| const announceToShell = useCallback( | ||
| async (workspace: { id: string; name: string }): Promise<void> => { | ||
| async (workspace: CreatedWorkspace): Promise<void> => { |
There was a problem hiding this comment.
New orgId provenance chain (workspaceEffects → workspaceSlice → NewWorkspaceForm) has no MFE-side test coverage
Severity: Minor
Problem
workspaceEffects.ts captures orgId from the create-requested closure and emits it on 'mfe/workspaces/created'; workspaceSlice.ts's new CreatedWorkspace type stores orgId in both created and workspaceAnnounceFailed; NewWorkspaceForm.tsx's announceToShell/retry path now reads workspace.orgId instead of a live useOrganization() value. None of workspaceEffects.ts, workspaceSlice.ts, or NewWorkspaceForm.tsx has an associated test file touched in this diff.
Reproduction, impact, suggested fix, verification
How to reproduce
- Inspect the diff for workspaceEffects.ts, workspaceSlice.ts, NewWorkspaceForm.tsx — all changed. 2. Search mfe_packages/projects-mfe/**/.test. for references to initWorkspaceEffects, CreatedWorkspace, workspaceAnnounceFailed, or NewWorkspaceForm — none found in the diff's changed-files list; only appContextEffects.test.ts (shell-side) was updated.
Expected behavior
New cross-boundary provenance logic (orgId capture and reuse on retry) should have MFE-side regression tests verifying the value threaded through effect → Redux state → retry publish is the organization the workspace was actually created under, especially across an org switch.
Actual behavior
Correctness of this chain currently rests entirely on manual code reading; a future edit (e.g., reverting to a live org read on retry, or dropping orgId in an action) would not be caught by any existing test.
create-requested(orgId) -> workspaceEffects (capture orgId, emit) -> workspaceSlice.created/workspaceAnnounceFailed(orgId) -> NewWorkspaceForm.announceToShell(workspace.orgId) -> publishCreatedWorkspace
^
no test exercises any of these links
Impact
A future regression re-introducing a live-organization read (instead of the captured orgId) or dropping the field during serialization would silently cause workspace-created announcements to be attributed to the wrong organization, with no CI signal.
Suggested correction
Add unit tests for initWorkspaceEffects verifying orgId is captured and emitted correctly, for workspaceSlice reducers verifying CreatedWorkspace.orgId round-trips through created/workspaceAnnounceFailed, and for NewWorkspaceForm verifying the retry path publishes the stored orgId (not a live organization value) after simulating an org switch.
How to verify
Run the new tests; they should fail if orgId capture/threading is reverted to a live-context read or if the field is dropped anywhere in the chain.
3 participants
No description provided.