lhumina_code/hero_os
5
0
Fork 0
Code Issues 30 Pull requests Projects 1 Releases Packages Wiki Activity Actions

shell: navigation to unknown island silently redirects (no 404 / not-found) #109

New issue
Closed
opened 2026-04-29 13:25:59 +00:00 by rawdaGastan · 4 comments
rawdaGastan commented 2026-04-29 13:25:59 +00:00
Member
Copy link

Problem

Direct navigation to a URL whose island id is not registered in this build silently rewrites the URL to the previously open island. No 404, no toast, no console error.

Repro

# Open these in order in the same tab:
# 1. http://127.0.0.1:9988/hero_os/ui/space/default/messaging  (works, populates active_context)
# 2. http://127.0.0.1:9988/hero_os/ui/space/default/room       (room island isn't compiled in this build)

Result after step 2: address bar reads …/space/default/messaging again — silent rewrite. Same for any other unknown id.

Why it matters

Users (and agents) following a stale or wrong URL get the impression nothing happened. Hard to debug — there is no message anywhere telling you the island id was unknown.

Suggested fix

Render an explicit 'Island "" not installed' empty state in the window for unknown ids instead of silently rerouting. Same affordance the Books / Voice / Whiteboard placeholders already use for not-installed services.

Found via QA session 2026-04-29 (qa/messaging_2026-04-29/FINDINGS.md).

## Problem Direct navigation to a URL whose island id is not registered in this build silently rewrites the URL to the previously open island. No 404, no toast, no console error. ## Repro ```bash # Open these in order in the same tab: # 1. http://127.0.0.1:9988/hero_os/ui/space/default/messaging (works, populates active_context) # 2. http://127.0.0.1:9988/hero_os/ui/space/default/room (room island isn't compiled in this build) ``` Result after step 2: address bar reads `…/space/default/messaging` again — silent rewrite. Same for any other unknown id. ## Why it matters Users (and agents) following a stale or wrong URL get the impression nothing happened. Hard to debug — there is no message anywhere telling you the island id was unknown. ## Suggested fix Render an explicit 'Island "<id>" not installed' empty state in the window for unknown ids instead of silently rerouting. Same affordance the Books / Voice / Whiteboard placeholders already use for not-installed services. Found via QA session 2026-04-29 (`qa/messaging_2026-04-29/FINDINGS.md`).
rawdaGastan added this to the ACTIVE project 2026-04-30 12:18:51 +00:00
rawdaGastan commented 2026-04-30 13:12:22 +00:00
Author
Member
Copy link

Implementation Spec for Issue #109

Objective

When a URL references an island id that has no metadata in the compiled-in registry, render an explicit "Island '' not installed" placeholder in a window for that id instead of silently dropping it and letting the URL get rewritten to the previously open island.

Investigation findings (the why)

The shell's URL → window translation lives in two places in crates/hero_os_app/src/main.rs:

  1. The first-load effect at main.rs:786-886: it parses the URL, then loops for (i, app_id) in route.apps.iter().enumerate() { if let Some(meta) = reg.get(app_id) { … new_windows.push(WindowState::new(…)) } else { tracing::warn!("URL references unknown island: {}", app_id); } } (lines 799-826). When every app id is unknown, new_windows stays empty, the if !new_windows.is_empty() guard at line 827 fails, control falls through to the localStorage branch, and the previously persisted desktop is restored.
  2. The popstate listener at main.rs:986-1037: same pattern — else if let Some(meta) = reg.get(app_id) at line 1009 silently skips unknown ids when rebuilding new_windows.

The "silent rewrite" the issue describes is then produced by a third effect — the window→URL sync at main.rs:910-954: it diffs windows.read() against the current URL; when windows doesn't contain the unknown id (because step 1/2 dropped it), it calls routing::replace_url(&route) with the actual window list, overwriting …/space/default/room with …/space/default/messaging.

The "knownness" comes from IslandRegistry in crates/hero_os_app/src/registry.rs: metadata for each island is gated by #[cfg(feature = "island-<id>")] (e.g. room_metadata at line 43), so an island id that's listed in hero_os_web/src/types.rs (e.g. island("room", …) at line 40) will not be in the runtime registry when the corresponding crate isn't compiled in.

The closest existing visual pattern is in crates/hero_os_app/src/island_content.rs:

  • Line 312-318: the settings fallback div { class: "island-placeholder", "Settings not available" }.
  • Line 609-614: the non-web-platform unknown-id arm div { class: "island-placeholder", "Island not available: {props.island_id}" }.

The fix is to drop the silent else skip on unknown ids in both main.rs spots, instead inserting a WindowState whose island_id keeps the unknown id verbatim so the IslandContent match falls to its existing _ / other arm and the window→URL sync sees the id in windows and stops rewriting.

Requirements

  • Direct navigation to a URL whose island id is unknown to the registry must create a window for that id, not skip it.
  • The window must render an explicit "Island '' not installed" placeholder (not a spinner, not a silent blank, not a fall-through to dynamic-loader fetch — that produces a misleading "Failed to load" message after a delay).
  • The URL bar must keep the unknown id; no auto-rewrite to a previously open island.
  • All known islands continue to load normally — no regression for the happy path.
  • The dock / sidebar must not change (the unknown id has no metadata, so it is naturally absent from the dock).
  • No new abstractions, no new components, no registry changes — reuse the existing island-placeholder div pattern from island_content.rs:312-318.

Files to Modify/Create

  • crates/hero_os_app/src/main.rs — replace silent else { warn } and silent else if … skip with WindowState creation that uses placeholder defaults for unknown ids (in both the first-load effect at lines 799-826 and the popstate handler at lines 1001-1024).
  • crates/hero_os_app/src/island_content.rs — replace the web-platform other fall-through (which currently delegates to DynamicIslandLoader) with a static "Island '' not installed" placeholder for the unknown-id case. Reuse the same island-placeholder div pattern used by the settings fallback at line 312-318.
  • crates/hero_os_app/src/routing.rs — add 1-2 unit tests covering parsing of an unknown id (the parser itself already does the right thing; the tests pin the contract that an unknown id stays in route.apps rather than being filtered).

Implementation Plan

Step 1: Stop dropping unknown ids on first URL load

Files: crates/hero_os_app/src/main.rs

  • In the first-load effect (around lines 799-826), replace the if let Some(meta) = reg.get(app_id) { … } else { tracing::warn!(…) } block with an unconditional push: when reg.get(app_id) is Some, use its default_size, name, default_view() (current behaviour). When it is None, push a WindowState::new with:
    • id = format!("{}-{}", app_id, i + 1) (same scheme),
    • island_id = app_id.clone() (preserve id verbatim — this is what the URL-sync effect compares against),
    • title = app_id.clone() (the placeholder body shows the id explicitly; title can also just be app_id so the WindowHeader has something to render),
    • width = 600.0, height = 400.0 (matches IslandMetadataBuilder's default, see hero_archipelagos/core/src/metadata.rs:410),
    • default_view = String::new().
  • Keep the existing tracing::warn!("URL references unknown island: {}", app_id) so we still get a console signal alongside the new visual.
  • Increment z regardless (move the z += 1 outside the if).

Dependencies: none.

Step 2: Stop dropping unknown ids on popstate

Files: crates/hero_os_app/src/main.rs

  • In the popstate closure (lines 997-1031), apply the same change: after the existing branch (line 1003), keep else if let Some(meta) = reg.get(app_id) for known ids, and add a final else branch that pushes a WindowState::new for the unknown id with the same placeholder defaults from Step 1. Title / size match Step 1.
  • Keep the same format!("{}-pop-{}", app_id, i) id naming so the popstate-created windows are clearly distinguishable.

Dependencies: Step 1 (same shape).

Step 3: Render an explicit "not installed" placeholder for unknown ids

Files: crates/hero_os_app/src/island_content.rs

  • Replace the web-platform other arm (currently delegating to DynamicIslandLoader) with a static "not installed" placeholder. Inspection of the codebase shows DynamicIslandLoader only fires for the unknown-id case (every known id has its own arm), and its only output is either a 404-Failed message or a JS-loaded custom element that has never been observed to succeed in default web builds (no /hero_os/ui/islands/<id>/...js files are produced by the default web build). This drops a code path that currently produces "Failed to load island: room" after a delay and replaces it with an immediate, accurate "Island 'room' not installed" message.
  • The web-platform other arm becomes: 
    #[cfg(feature = "web-platform")]
other => rsx! {
    div {
        class: "island-placeholder",
        style: "display: flex; flex-direction: column; align-items: center; justify-content: center; gap: 8px; height: 100%; padding: 24px; text-align: center;",
        div { style: "font-weight: 600;", "Island \"{other}\" not installed" }
        div {
            class: "island-placeholder-detail",
            style: "color: var(--color-text-muted); font-size: 0.9em;",
            "This island is not part of the current Hero OS build."
        }
    }
},
    And the non-web-platform arm at line 609-614 already says "Island not available: {props.island_id}" — update its copy to match for consistency: "Island \"{props.island_id}\" not installed".
  • Verify dynamic_loader.rs is not referenced elsewhere; if it is now dead, leave the file in place but add a #[allow(dead_code)] at the module level (do not delete — the repo intentionally keeps the dynamic-loader machinery for future runtime island distribution; deletion is out of scope for this issue).

Dependencies: none. Independent from Steps 1-2 but must land together for the URL-bar fix to also produce a visible placeholder.

Step 4: Tests

Files: crates/hero_os_app/src/routing.rs

  • Add two pure-parser tests that pin the contract:
    • test_parse_unknown_island_id_kept_in_apps: parse_url("/space/default/room", "") → route.apps == vec!["room"] regardless of whether room is "real". This test belongs in routing.rs because that module is the seam — it documents that the parser is registry-agnostic.
    • test_parse_mixed_known_and_unknown: parse_url("/space/default/messaging+room", "") → route.apps == vec!["messaging", "room"].
  • These tests already pass with current code (parsing is registry-agnostic), but they pin the contract so a future "filter at parse time" refactor would fail loudly.

Dependencies: none.

Acceptance Criteria

  • Direct navigation to /hero_os/ui/space/default/room (or any unknown id) renders an explicit "Island '' not installed" empty state in the window.
  • URL is NOT auto-rewritten to the previously open island.
  • Browser back/forward (popstate) for an unknown id also produces the placeholder window, no rewrite.
  • Existing islands continue to load normally — no regression.
  • cargo check -p hero_os_app and cargo test -p hero_os_app still pass (baseline confirmed clean).
  • The tracing::warn!("URL references unknown island: {}", app_id) log still fires so the console signal mentioned in the issue body is produced.
  • Two new pure parser tests added in routing.rs documenting that unknown ids stay in route.apps.
  • Dock / sidebar unchanged — unknown ids do not appear in the dock (they have no metadata, so they were never there).

Notes

  • Mixed known + unknown ids: a URL like /space/default/messaging+room opens two windows: a working messaging window and a placeholder room window. This falls out naturally from the per-id loop change in Step 1; verify in the smoke test.
  • Save state pollution: save_desktop_state (main.rs:889-902) will persist the placeholder window into localStorage. That means after a refresh the placeholder is restored even without the URL hint — desirable, since the user explicitly opened it and the URL still reflects it. If the user closes the placeholder window, normal close handling (controller.rs:152-166) cleans it up via the push_url_state call.
  • Window title: using app_id as the title is acceptable; the placeholder body re-shows the id so the user can copy-paste it. If the team wants a prettier title, capitalising the first letter is a one-liner — but per the "no new abstractions" constraint, app_id verbatim is fine.
  • Why not gate the URL parser to only known ids: doing the filter in parse_url would couple the routing module to the registry signal (currently it has zero crate-internal dependencies — it's a pure function with #[cfg] shims). The existing layering is correct: parse → consult registry at the use site → render. The fix preserves that.
  • Why not delete dynamic_loader.rs: the runtime dynamic-load story is a documented future direction in the file's docstring; the bug fix only needs to bypass it for the unknown-id case. Leaving it in place keeps the spec narrowly scoped.
  • Mobile (#[cfg(not(feature = "web-platform"))]): the existing arm at island_content.rs:609-614 already shows a placeholder string. Step 3 just normalises the wording. Mobile does not have the silent-rewrite bug because its initial_context is hard-coded to "geomind" (main.rs:247) and there is no popstate/URL-sync effect block (#[cfg(feature = "web-platform")] gate in main.rs:300), so Steps 1-2 are also web-only by construction (they live inside the same #[cfg(feature = "web-platform")] block — verify the modifications stay inside it).
## Implementation Spec for Issue #109 ### Objective When a URL references an island id that has no metadata in the compiled-in registry, render an explicit "Island '<id>' not installed" placeholder in a window for that id instead of silently dropping it and letting the URL get rewritten to the previously open island. ### Investigation findings (the why) The shell's URL → window translation lives in two places in `crates/hero_os_app/src/main.rs`: 1. The **first-load effect** at `main.rs:786-886`: it parses the URL, then loops `for (i, app_id) in route.apps.iter().enumerate() { if let Some(meta) = reg.get(app_id) { … new_windows.push(WindowState::new(…)) } else { tracing::warn!("URL references unknown island: {}", app_id); } }` (lines 799-826). When every app id is unknown, `new_windows` stays empty, the `if !new_windows.is_empty()` guard at line 827 fails, control falls through to the localStorage branch, and the previously persisted desktop is restored. 2. The **popstate listener** at `main.rs:986-1037`: same pattern — `else if let Some(meta) = reg.get(app_id)` at line 1009 silently skips unknown ids when rebuilding `new_windows`. The "silent rewrite" the issue describes is then produced by a third effect — the **window→URL sync** at `main.rs:910-954`: it diffs `windows.read()` against the current URL; when `windows` doesn't contain the unknown id (because step 1/2 dropped it), it calls `routing::replace_url(&route)` with the actual window list, overwriting `…/space/default/room` with `…/space/default/messaging`. The "knownness" comes from `IslandRegistry` in `crates/hero_os_app/src/registry.rs`: `metadata` for each island is gated by `#[cfg(feature = "island-<id>")]` (e.g. `room_metadata` at line 43), so an island id that's listed in `hero_os_web/src/types.rs` (e.g. `island("room", …)` at line 40) will not be in the runtime registry when the corresponding crate isn't compiled in. The closest existing visual pattern is in `crates/hero_os_app/src/island_content.rs`: - Line 312-318: the settings fallback `div { class: "island-placeholder", "Settings not available" }`. - Line 609-614: the non-`web-platform` unknown-id arm `div { class: "island-placeholder", "Island not available: {props.island_id}" }`. The fix is to drop the silent `else` skip on unknown ids in both main.rs spots, instead inserting a `WindowState` whose `island_id` keeps the unknown id verbatim so the `IslandContent` match falls to its existing `_` / `other` arm and the window→URL sync sees the id in `windows` and stops rewriting. ### Requirements - Direct navigation to a URL whose island id is unknown to the registry must create a window for that id, not skip it. - The window must render an explicit "Island '<id>' not installed" placeholder (not a spinner, not a silent blank, not a fall-through to dynamic-loader fetch — that produces a misleading "Failed to load" message after a delay). - The URL bar must keep the unknown id; no auto-rewrite to a previously open island. - All known islands continue to load normally — no regression for the happy path. - The dock / sidebar must not change (the unknown id has no metadata, so it is naturally absent from the dock). - No new abstractions, no new components, no registry changes — reuse the existing `island-placeholder` div pattern from `island_content.rs:312-318`. ### Files to Modify/Create - `crates/hero_os_app/src/main.rs` — replace silent `else { warn }` and silent `else if … skip` with `WindowState` creation that uses placeholder defaults for unknown ids (in both the first-load effect at lines 799-826 and the popstate handler at lines 1001-1024). - `crates/hero_os_app/src/island_content.rs` — replace the `web-platform` `other` fall-through (which currently delegates to `DynamicIslandLoader`) with a static "Island '<id>' not installed" placeholder for the unknown-id case. Reuse the same `island-placeholder` div pattern used by the settings fallback at line 312-318. - `crates/hero_os_app/src/routing.rs` — add 1-2 unit tests covering parsing of an unknown id (the parser itself already does the right thing; the tests pin the contract that an unknown id stays in `route.apps` rather than being filtered). ### Implementation Plan #### Step 1: Stop dropping unknown ids on first URL load Files: `crates/hero_os_app/src/main.rs` - In the first-load effect (around lines 799-826), replace the `if let Some(meta) = reg.get(app_id) { … } else { tracing::warn!(…) }` block with an unconditional push: when `reg.get(app_id)` is `Some`, use its `default_size`, `name`, `default_view()` (current behaviour). When it is `None`, push a `WindowState::new` with: - `id = format!("{}-{}", app_id, i + 1)` (same scheme), - `island_id = app_id.clone()` (preserve id verbatim — this is what the URL-sync effect compares against), - `title = app_id.clone()` (the placeholder body shows the id explicitly; title can also just be `app_id` so the WindowHeader has something to render), - `width = 600.0, height = 400.0` (matches `IslandMetadataBuilder`'s default, see `hero_archipelagos/core/src/metadata.rs:410`), - `default_view = String::new()`. - Keep the existing `tracing::warn!("URL references unknown island: {}", app_id)` so we still get a console signal alongside the new visual. - Increment `z` regardless (move the `z += 1` outside the `if`). Dependencies: none. #### Step 2: Stop dropping unknown ids on popstate Files: `crates/hero_os_app/src/main.rs` - In the popstate closure (lines 997-1031), apply the same change: after the `existing` branch (line 1003), keep `else if let Some(meta) = reg.get(app_id)` for known ids, and add a final `else` branch that pushes a `WindowState::new` for the unknown id with the same placeholder defaults from Step 1. Title / size match Step 1. - Keep the same `format!("{}-pop-{}", app_id, i)` id naming so the popstate-created windows are clearly distinguishable. Dependencies: Step 1 (same shape). #### Step 3: Render an explicit "not installed" placeholder for unknown ids Files: `crates/hero_os_app/src/island_content.rs` - Replace the `web-platform` `other` arm (currently delegating to `DynamicIslandLoader`) with a static "not installed" placeholder. Inspection of the codebase shows `DynamicIslandLoader` only fires for the unknown-id case (every known id has its own arm), and its only output is either a 404-Failed message or a JS-loaded custom element that has never been observed to succeed in default web builds (no `/hero_os/ui/islands/<id>/...js` files are produced by the default `web` build). This drops a code path that currently produces "Failed to load island: room" after a delay and replaces it with an immediate, accurate "Island 'room' not installed" message. - The `web-platform` `other` arm becomes: ``` #[cfg(feature = "web-platform")] other => rsx! { div { class: "island-placeholder", style: "display: flex; flex-direction: column; align-items: center; justify-content: center; gap: 8px; height: 100%; padding: 24px; text-align: center;", div { style: "font-weight: 600;", "Island \"{other}\" not installed" } div { class: "island-placeholder-detail", style: "color: var(--color-text-muted); font-size: 0.9em;", "This island is not part of the current Hero OS build." } } }, ``` And the non-`web-platform` arm at line 609-614 already says `"Island not available: {props.island_id}"` — update its copy to match for consistency: `"Island \"{props.island_id}\" not installed"`. - Verify `dynamic_loader.rs` is not referenced elsewhere; if it is now dead, leave the file in place but add a `#[allow(dead_code)]` at the module level (do not delete — the repo intentionally keeps the dynamic-loader machinery for future runtime island distribution; deletion is out of scope for this issue). Dependencies: none. Independent from Steps 1-2 but must land together for the URL-bar fix to also produce a visible placeholder. #### Step 4: Tests Files: `crates/hero_os_app/src/routing.rs` - Add two pure-parser tests that pin the contract: - `test_parse_unknown_island_id_kept_in_apps`: `parse_url("/space/default/room", "")` → `route.apps == vec!["room"]` regardless of whether `room` is "real". This test belongs in routing.rs because that module is the seam — it documents that the parser is registry-agnostic. - `test_parse_mixed_known_and_unknown`: `parse_url("/space/default/messaging+room", "")` → `route.apps == vec!["messaging", "room"]`. - These tests already pass with current code (parsing is registry-agnostic), but they pin the contract so a future "filter at parse time" refactor would fail loudly. Dependencies: none. ### Acceptance Criteria - [ ] Direct navigation to `/hero_os/ui/space/default/room` (or any unknown id) renders an explicit "Island '<id>' not installed" empty state in the window. - [ ] URL is NOT auto-rewritten to the previously open island. - [ ] Browser back/forward (popstate) for an unknown id also produces the placeholder window, no rewrite. - [ ] Existing islands continue to load normally — no regression. - [ ] `cargo check -p hero_os_app` and `cargo test -p hero_os_app` still pass (baseline confirmed clean). - [ ] The `tracing::warn!("URL references unknown island: {}", app_id)` log still fires so the console signal mentioned in the issue body is produced. - [ ] Two new pure parser tests added in `routing.rs` documenting that unknown ids stay in `route.apps`. - [ ] Dock / sidebar unchanged — unknown ids do not appear in the dock (they have no metadata, so they were never there). ### Notes - **Mixed known + unknown ids**: a URL like `/space/default/messaging+room` opens two windows: a working messaging window and a placeholder room window. This falls out naturally from the per-id loop change in Step 1; verify in the smoke test. - **Save state pollution**: `save_desktop_state` (`main.rs:889-902`) will persist the placeholder window into localStorage. That means after a refresh the placeholder is restored even without the URL hint — desirable, since the user explicitly opened it and the URL still reflects it. If the user closes the placeholder window, normal close handling (`controller.rs:152-166`) cleans it up via the `push_url_state` call. - **Window title**: using `app_id` as the title is acceptable; the placeholder body re-shows the id so the user can copy-paste it. If the team wants a prettier title, capitalising the first letter is a one-liner — but per the "no new abstractions" constraint, `app_id` verbatim is fine. - **Why not gate the URL parser to only known ids**: doing the filter in `parse_url` would couple the routing module to the registry signal (currently it has zero crate-internal dependencies — it's a pure function with `#[cfg]` shims). The existing layering is correct: parse → consult registry at the use site → render. The fix preserves that. - **Why not delete `dynamic_loader.rs`**: the runtime dynamic-load story is a documented future direction in the file's docstring; the bug fix only needs to bypass it for the unknown-id case. Leaving it in place keeps the spec narrowly scoped. - **Mobile** (`#[cfg(not(feature = "web-platform"))]`): the existing arm at `island_content.rs:609-614` already shows a placeholder string. Step 3 just normalises the wording. Mobile does not have the silent-rewrite bug because its `initial_context` is hard-coded to `"geomind"` (`main.rs:247`) and there is no `popstate`/URL-sync effect block (`#[cfg(feature = "web-platform")]` gate in `main.rs:300`), so Steps 1-2 are also web-only by construction (they live inside the same `#[cfg(feature = "web-platform")]` block — verify the modifications stay inside it).
rawdaGastan commented 2026-04-30 14:36:47 +00:00
Author
Member
Copy link

Test Results

Static checks

  • cargo check -p hero_os_app — clean.
  • make check (workspace, all crates) — clean, finished in 25.18s.
  • cargo test -p hero_os_app — 42 passed, 0 failed, 0 ignored. Includes the two new parser tests:
    • routing::tests::test_parse_unknown_island_id_kept_in_apps — pass
    • routing::tests::test_parse_mixed_known_and_unknown — pass

End-to-end browser smoke (via Hero Browser MCP)

Stopped + rebuilt + reinstalled hero_os via make run, then drove a headless Chromium against the live UI:

  1. Navigate to http://127.0.0.1:9988/hero_os/ui/space/default/messaging — messaging island loads, baseline established.
  2. Navigate to http://127.0.0.1:9988/hero_os/ui/space/default/room — found that room IS compiled in this build (the Conference island opened). The issue body's specific repro id is no longer unknown on development head.
  3. Navigate to http://127.0.0.1:9988/hero_os/ui/space/default/nonexistent_island_xyz (a guaranteed-unknown id):
    • URL preserved verbatim (no auto-rewrite to messaging) ✓
    • A window opened with title nonexistent_island_xyz ✓
    • The window body renders: Island "nonexistent_island_xyz" not installed followed by This island is not part of the current Hero OS build. ✓
  4. Browser back: returns to …/room (Conference window) — works ✓
  5. Browser forward (popstate): returns to …/nonexistent_island_xyz — placeholder window renders again, URL preserved ✓

All acceptance criteria from the spec are met:

  • Direct navigation to an unknown id → explicit "not installed" placeholder
  • URL is NOT auto-rewritten
  • Browser back/forward (popstate) for an unknown id also produces the placeholder window with URL preserved
  • Existing islands (messaging, Conference) load normally — no regression
  • cargo check and cargo test pass

Note on the issue's original repro id

The issue body uses room as the unknown-id example. Between the issue being filed (2026-04-29) and this fix (2026-04-30), room (Conference / livekit) is now compiled in by default in the workspace build, so it's no longer a useful repro id. The behaviour the fix addresses is identical — verified with nonexistent_island_xyz instead. Anything that's not in the runtime registry hits the new placeholder.

## Test Results ### Static checks - `cargo check -p hero_os_app` — clean. - `make check` (workspace, all crates) — clean, finished in 25.18s. - `cargo test -p hero_os_app` — 42 passed, 0 failed, 0 ignored. Includes the two new parser tests: - `routing::tests::test_parse_unknown_island_id_kept_in_apps` — pass - `routing::tests::test_parse_mixed_known_and_unknown` — pass ### End-to-end browser smoke (via Hero Browser MCP) Stopped + rebuilt + reinstalled hero_os via `make run`, then drove a headless Chromium against the live UI: 1. Navigate to `http://127.0.0.1:9988/hero_os/ui/space/default/messaging` — messaging island loads, baseline established. 2. Navigate to `http://127.0.0.1:9988/hero_os/ui/space/default/room` — found that `room` IS compiled in this build (the Conference island opened). The issue body's specific repro id is no longer unknown on `development` head. 3. Navigate to `http://127.0.0.1:9988/hero_os/ui/space/default/nonexistent_island_xyz` (a guaranteed-unknown id): - URL preserved verbatim (no auto-rewrite to messaging) ✓ - A window opened with title `nonexistent_island_xyz` ✓ - The window body renders: `Island "nonexistent_island_xyz" not installed` followed by `This island is not part of the current Hero OS build.` ✓ 4. Browser back: returns to `…/room` (Conference window) — works ✓ 5. Browser forward (popstate): returns to `…/nonexistent_island_xyz` — placeholder window renders again, URL preserved ✓ All acceptance criteria from the spec are met: - Direct navigation to an unknown id → explicit "not installed" placeholder - URL is NOT auto-rewritten - Browser back/forward (popstate) for an unknown id also produces the placeholder window with URL preserved - Existing islands (messaging, Conference) load normally — no regression - `cargo check` and `cargo test` pass ### Note on the issue's original repro id The issue body uses `room` as the unknown-id example. Between the issue being filed (2026-04-29) and this fix (2026-04-30), `room` (Conference / livekit) is now compiled in by default in the workspace build, so it's no longer a useful repro id. The behaviour the fix addresses is identical — verified with `nonexistent_island_xyz` instead. Anything that's not in the runtime registry hits the new placeholder.
rawdaGastan commented 2026-04-30 14:37:44 +00:00
Author
Member
Copy link

Implementation Summary

Changes

  • crates/hero_os_app/src/main.rs — in BOTH the first-load URL effect and the popstate listener, the else branch that previously logged a warning and silently dropped unknown island ids now also pushes a WindowState for the unknown id (id verbatim, title = id, default 600x400 size). Window→URL sync therefore sees the id in the windows list and stops rewriting the URL.
  • crates/hero_os_app/src/island_content.rs — replaced the web-platform other arm (which delegated to DynamicIslandLoader and produced a misleading "Failed to load" after a delay) with a static island-placeholder div that renders Island "<id>" not installed plus a clarifying detail line. Updated the not(web-platform) arm copy to match for consistency.
  • crates/hero_os_app/src/dynamic_loader.rs — added module-level #![allow(dead_code)] since DynamicIslandLoader is no longer wired into any caller. Module is preserved per the spec for future runtime island distribution.
  • crates/hero_os_app/src/routing.rs — added two pure-parser unit tests (test_parse_unknown_island_id_kept_in_apps, test_parse_mixed_known_and_unknown) that pin the contract that parse_url is registry-agnostic and keeps unknown ids in route.apps.

Why

The shell's URL → window translation in two effects (first-load + popstate) was using if let Some(meta) = reg.get(app_id) { … } else { drop }. When a URL referenced an island id not in the runtime registry (e.g. a feature-gated island that isn't compiled in this build), the resulting windows list was empty for that id, then the window→URL sync effect rewrote the URL to match the actual (previously open) window list — producing the silent rewrite the issue reports. Pushing a WindowState for the unknown id keeps the URL stable and lets the existing _ arm in IslandContent render an explicit placeholder.

Test results

See test results comment.

  • Static checks: cargo check, make check, cargo test — all clean.
  • 42 tests passed (2 new), 0 failures.
  • End-to-end browser smoke via Hero Browser MCP: unknown id renders placeholder, URL preserved, popstate behaves correctly, no regression on known islands.

Caveats

  • The issue body's repro id room is now compiled in by default on development head (the Conference island ships in the build), so it's no longer a useful repro id. The fix was verified with nonexistent_island_xyz instead — the underlying behaviour is identical for any id not in the runtime registry.
  • The previously-installed window state can persist in localStorage for the unknown-id placeholder window. If the user closes that window, normal close handling cleans it up. If they refresh while it's open, it's restored from localStorage — desirable, since the URL still references it.
  • dynamic_loader.rs is now dead at the crate level. Left in place per the spec; can be removed in a follow-up if/when the team decides to drop the runtime-load story entirely.
## Implementation Summary ### Changes - `crates/hero_os_app/src/main.rs` — in BOTH the first-load URL effect and the popstate listener, the `else` branch that previously logged a warning and silently dropped unknown island ids now also pushes a `WindowState` for the unknown id (id verbatim, title = id, default 600x400 size). Window→URL sync therefore sees the id in the windows list and stops rewriting the URL. - `crates/hero_os_app/src/island_content.rs` — replaced the `web-platform` `other` arm (which delegated to `DynamicIslandLoader` and produced a misleading "Failed to load" after a delay) with a static `island-placeholder` div that renders `Island "<id>" not installed` plus a clarifying detail line. Updated the `not(web-platform)` arm copy to match for consistency. - `crates/hero_os_app/src/dynamic_loader.rs` — added module-level `#![allow(dead_code)]` since `DynamicIslandLoader` is no longer wired into any caller. Module is preserved per the spec for future runtime island distribution. - `crates/hero_os_app/src/routing.rs` — added two pure-parser unit tests (`test_parse_unknown_island_id_kept_in_apps`, `test_parse_mixed_known_and_unknown`) that pin the contract that `parse_url` is registry-agnostic and keeps unknown ids in `route.apps`. ### Why The shell's URL → window translation in two effects (first-load + popstate) was using `if let Some(meta) = reg.get(app_id) { … } else { drop }`. When a URL referenced an island id not in the runtime registry (e.g. a feature-gated island that isn't compiled in this build), the resulting `windows` list was empty for that id, then the window→URL sync effect rewrote the URL to match the actual (previously open) window list — producing the silent rewrite the issue reports. Pushing a `WindowState` for the unknown id keeps the URL stable and lets the existing `_` arm in `IslandContent` render an explicit placeholder. ### Test results See [test results comment](https://forge.ourworld.tf/lhumina_code/hero_os/issues/109#issuecomment-27732). - Static checks: `cargo check`, `make check`, `cargo test` — all clean. - 42 tests passed (2 new), 0 failures. - End-to-end browser smoke via Hero Browser MCP: unknown id renders placeholder, URL preserved, popstate behaves correctly, no regression on known islands. ### Caveats - The issue body's repro id `room` is now compiled in by default on `development` head (the Conference island ships in the build), so it's no longer a useful repro id. The fix was verified with `nonexistent_island_xyz` instead — the underlying behaviour is identical for any id not in the runtime registry. - The previously-installed window state can persist in `localStorage` for the unknown-id placeholder window. If the user closes that window, normal close handling cleans it up. If they refresh while it's open, it's restored from localStorage — desirable, since the URL still references it. - `dynamic_loader.rs` is now dead at the crate level. Left in place per the spec; can be removed in a follow-up if/when the team decides to drop the runtime-load story entirely.
rawdaGastan commented 2026-04-30 14:41:00 +00:00
Author
Member
Copy link

Pull request opened: #122

This PR implements the placeholder-window approach from the spec — unknown island ids no longer trigger the silent URL rewrite; instead a window opens with the id verbatim and renders an explicit Island "<id>" not installed placeholder. URL is preserved on direct navigation and on browser back/forward.

Pull request opened: https://forge.ourworld.tf/lhumina_code/hero_os/pulls/122 This PR implements the placeholder-window approach from the spec — unknown island ids no longer trigger the silent URL rewrite; instead a window opens with the id verbatim and renders an explicit `Island "<id>" not installed` placeholder. URL is preserved on direct navigation and on browser back/forward.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
development
main
v0.1.3
v0.1.2
v0.1.1
v0.1.0
v0.0.1-rc1
Labels
Clear labels   
prio_critical

   
prio_low

   
type_bug

   
type_contact

   
type_issue

   
type_lead

   
type_question

   
type_story

   
type_task
No labels
prio_critical
prio_low
type_bug
type_contact
type_issue
type_lead
type_question
type_story
type_task
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
ACTIVE
Assignees
Clear assignees
No assignees
rawdaGastan
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
lhumina_code/hero_os#109
No description provided.