Skip to content

refactor: isolate workspace embed shell#264

Open
mariusvniekerk wants to merge 3 commits intofix-kata-54-nested-embed-repo-pathfrom
fix-kata-53-embeddable-app-shell
Open

refactor: isolate workspace embed shell#264
mariusvniekerk wants to merge 3 commits intofix-kata-54-nested-embed-repo-pathfrom
fix-kata-53-embeddable-app-shell

Conversation

@mariusvniekerk
Copy link
Copy Markdown
Collaborator

@mariusvniekerk mariusvniekerk commented May 7, 2026

Embed routes now render through a smaller shell that skips standalone settings startup, sync/event preload behavior, sidebar/container setup, and global shortcuts. The embed contract documents one isolated browsing context per Middleman embed instance.

@mariusvniekerk
Copy link
Copy Markdown
Collaborator Author

mariusvniekerk commented May 7, 2026 •
edited
Loading

This change is part of the following stack:

Change managed by git-spice.

This was referenced May 7, 2026
Open
Open
Open
Open
@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 7, 2026

roborev: Combined Review (afb0506)

High-risk issues remain in the workspace embed shell refactor, mainly around missing provider wiring and embed startup behavior.

High

  • frontend/src/lib/components/terminal/WorkspaceEmbedShell.svelte:57
    • Problem: The embedded <Provider> no longer passes onNavigate, so components relying on provider navigation context now receive the default no-op. Internal navigation in embedded views, such as PR detail links or sidebar actions, can silently stop working.
    • Fix: Pass the navigation handler into the provider, e.g. onNavigate={(e) => navigate(typeof e === "string" ? e : e.path)}.

Medium

  • frontend/src/lib/components/terminal/WorkspaceEmbedShell.svelte:57

    • Problem: The embedded <Provider> is also missing hostState. Components that read host state from context, such as helpers for global repo or active worktree state, may fail or behave incorrectly in embed mode.
    • Fix: Provide hostState to the provider, or supply a safe embed-specific stub for the expected methods.

  • frontend/src/App.svelte:434

    • Problem: Shell selection happens before embed.initialRoute is applied in onMount. If a host loads the base document and relies on initialRoute to select an embed route, the first render can still mount the full app provider and side effects before switching to WorkspaceEmbedShell.
    • Fix: Apply or account for initialRoute before the first shell selection render so embed initial routes mount the embed shell immediately.

  • frontend/src/lib/utils/appShell.test.ts:5

    • Problem: The change only adds a unit test for the shell predicate, but this refactor affects user-visible startup, routing, and workspace embed surfaces. The repository guidelines call for e2e coverage for these flows.
    • Fix: Add e2e coverage that loads representative workspace embed routes through the real HTTP API and SQLite path, verifying the embed surface renders without the full app shell.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 7, 2026

roborev: Combined Review (bbcfa1d)

No Medium, High, or Critical issues found.

All review agents reported the code as clean.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@mariusvniekerk mariusvniekerk force-pushed the fix-kata-54-nested-embed-repo-path branch from fd72bea to 974f6f5 Compare May 7, 2026 13:09
@mariusvniekerk mariusvniekerk force-pushed the fix-kata-53-embeddable-app-shell branch from bbcfa1d to 10276f6 Compare May 7, 2026 13:09
@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 7, 2026

roborev: Combined Review (10276f6)

No Medium-or-higher issues found.

All completed reviews report the code is clean.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 7, 2026

roborev: Combined Review (10276f6)

No Medium, High, or Critical issues found.

All reviewed agents reported the code as clean.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@mariusvniekerk mariusvniekerk force-pushed the fix-kata-54-nested-embed-repo-path branch from 974f6f5 to 8e6fd1e Compare May 7, 2026 13:16
@mariusvniekerk mariusvniekerk force-pushed the fix-kata-53-embeddable-app-shell branch from 10276f6 to e03f9df Compare May 7, 2026 13:16
@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 7, 2026

roborev: Combined Review (e03f9df)

Medium issue found; security review found no exploitable issues.

Medium

  • frontend/src/App.svelte:91 - Initializing on an embed route skips full-app startup permanently, but the route can later switch to a standalone full-app route like /terminal, /workspaces, or /pulls. Since onMount will not rerun, appReady stays false and the full shell never hydrates settings/stores or starts normal data flow.

    Fix: Lazily run full-shell initialization when the route first becomes non-embed, or prevent embed surfaces from navigating into standalone routes and delegate that navigation to the host.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@mariusvniekerk
refactor: isolate workspace embed shell
4e3b739 
Embed routes now render through a smaller shell that skips standalone settings startup, sync/event preload behavior, sidebar/container setup, and global shortcuts. The embed contract documents one isolated browsing context per Middleman embed instance.
@mariusvniekerk
refactor: simplify workspace embed branch code
2857b26 
The workspace embed branch had several duplicated conversion and routing paths that made the new surfaces harder to audit. This keeps the behavior and API contracts intact while giving the project scanners, response mappers, route page classification, and tooling-status rendering a single expression of the same logic.

The commit hook also exposed that project identity resolution could inherit Git hook environment and read the parent worktree remote instead of the target checkout. Sanitize child git processes with the existing gitenv helper so project registration stays scoped to the requested path under hook-driven tests too.

Validation:
- go test ./internal/db -shuffle=on
- go test ./internal/projects -shuffle=on
- GIT_DIR=/tmp/poison.git GIT_WORK_TREE=/tmp/poison go test ./internal/projects -shuffle=on
- go test ./internal/server -run '^(TestW1SliceAGate|TestRegisterProject_|TestGetProject_|TestRegisterWorktree_|TestListProjects_|TestListLaunchTargets_)' -shuffle=on\n- bun run --cwd frontend typecheck\n- bun run --cwd frontend lint\n- bun run --cwd frontend test src/lib/stores/router.test.ts src/lib/components/terminal/ToolingStatusBlock.test.ts src/lib/components/terminal/WorkspaceProjectCard.test.ts\n- bunx @sveltejs/mcp@0.1.22 svelte-autofixer frontend/src/App.svelte --svelte-version 5 (no issues; broad pre-existing effect suggestions only)\n- bunx @sveltejs/mcp@0.1.22 svelte-autofixer frontend/src/lib/components/terminal/ToolingStatusBlock.svelte --svelte-version 5\n- bunx @sveltejs/mcp@0.1.22 svelte-autofixer frontend/src/lib/components/terminal/WorkspaceProjectCard.svelte --svelte-version 5\n- pre-commit hook path including go test (short)\n\nGenerated with OpenAI Codex\nCo-authored-by: OpenAI Codex <noreply@openai.com>
@mariusvniekerk
fix: start full app after embed navigation
1cf8b79 
Embedded startup can begin on an embed-only route and later navigate into the standalone app; lazily starting the full shell against the bound full-app stores keeps normal data loading active.
@mariusvniekerk mariusvniekerk force-pushed the fix-kata-53-embeddable-app-shell branch from e03f9df to 1cf8b79 Compare May 8, 2026 01:28
@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented May 8, 2026

roborev: Combined Review (1cf8b79)

Medium issue found; no Critical or High findings.

Medium

  • frontend/src/App.svelte:93 - cleanupFullAppShell remains set after startFullAppShell runs, so navigating full app -> embed -> full app can remount Provider with new StoreInstances while startup is skipped. The full app may render with unhydrated settings and without sync polling/event connection, while old stores may remain connected.

    Fix: Run and clear the full-shell cleanup when leaving the full shell, or make startFullAppShell detect changed StoreInstances, clean up the old shell, and start the new one. Add e2e coverage for full app -> embed -> full app navigation.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@mariusvniekerk