Conversation
Each widget card header gains an anchor to the widget's full-page twin: /projects/<path>, /full/apps/<appId>/<route>, or /desktop. The selection broadcast now reports the folder the browser is showing, so the Projects placement persists folders as well as files and the link carries them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Jessie-QingYu
left a comment
There was a problem hiding this comment.
Code Review: ✅ APPROVE
This PR adds an "Open in new tab" anchor to each desktop widget card header that links to the widget's full-page twin (/projects/<path>, /desktop, or /full/apps/<appId>/<route>?<params>). It extracts the app-path construction into widget-links.ts (buildFullAppPath/getWidgetFullHref) shared by both the iframe src and the anchor, widens the file-browser selection broadcast with currentFolderPath so folder drilling is persisted onto the projects placement, and adds the missing aria-label to the close button. I fully verified the touched files: the AppWidget src now delegates to buildFullAppPath with logic identical to the removed inline code, the anchor re-reads the live iframe URL via the existing resolveLink (which encodes/decodes routes symmetrically), and getWidgetFullHref correctly returns null for chat and target-less app tiles. updateProjectsSelection no-ops on unchanged paths, and ProjectsWidget is the only consumer of the widened callback, so the type change is fully contained.
Security is sound: all hrefs are internal, root-anchored paths with per-segment encodeURIComponent (including # -> %23, blocking fragment injection), target="_blank" carries rel="noopener noreferrer", and cross-origin iframe reads are guarded with try/catch. Only minor, non-blocking observations remain.
Verdict: APPROVE — Clean, well-tested feature with a shared link builder that prevents drift; no correctness, security, or design regressions found.
2 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P3 | performance | packages/web/src/pages/free/FreeGrid.tsx |
refreshFullHref useCallback depends on whole widget object |
| P3 | code-quality | packages/web/src/pages/free/ProjectsWidget.tsx |
Projects href does not update when a file is open and the pane drills folders |
| const registry = useWorkspaceContextRegistry(); | ||
| const [liveAppHref, setLiveAppHref] = useState<string | null>(null); | ||
| const fullHref = liveAppHref ?? getWidgetFullHref(widget); | ||
| const refreshFullHref = useCallback(() => { |
There was a problem hiding this comment.
[P3] performance — refreshFullHref useCallback depends on whole widget object
refreshFullHref lists widget in its dependency array. Because the placement object is typically a fresh reference each render, the useCallback is effectively recreated every render, negating memoization and forcing the anchor's onPointerEnter/onFocus handlers to change identity. Consider depending on the specific primitives used (widget.type, widget.id, widget.targetId). Purely a minor perf/cleanliness nit.
| // browser is showing when no file is open. The restore path already | ||
| // resolves either kind through `/resolve`. | ||
| if (placementId) { | ||
| updateProjectsSelection(placementId, sel.selectedPath ?? sel.currentFolderPath); |
There was a problem hiding this comment.
[P3] code-quality — Projects href does not update when a file is open and the pane drills folders
updateProjectsSelection(placementId, sel.selectedPath ?? sel.currentFolderPath) prioritizes the open file, so drilling into a different folder while a file is still selected leaves the persisted selectedPath (and the new-tab link) pointing at the old file rather than the folder now shown. This matches the stated "open file, or folder when no file is open" design, so it is acceptable — flagging only so the intent is explicit and confirmed.
yunfanye
left a comment
There was a problem hiding this comment.
Code Review: 🛑 REQUEST_CHANGES
This PR cleanly centralizes full-app URL construction and adds native new-tab links while persisting the Projects pane's current folder. The widened placement semantics are not propagated to the public shared-projects view, however, so a newly persisted folder is deterministically restored through the file-loading path in shared chats.
I inspected the full changed files and traced the file-browser selection, workspace registry, full-app routing, placement persistence, and share consumers. I could not rerun the web typecheck/tests because the provided clone has no installed dependencies (tsc: not found).
Verdict: REQUEST_CHANGES — The new folder-valued placement state breaks restoration of Projects widgets in public shared chats and should be fixed before merge.
1 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P1 | error-handling | packages/web/src/pages/free/ProjectsWidget.tsx |
Restore shared folder placements as directories |
| // browser is showing when no file is open. The restore path already | ||
| // resolves either kind through `/resolve`. | ||
| if (placementId) { | ||
| updateProjectsSelection(placementId, sel.selectedPath ?? sel.currentFolderPath); |
There was a problem hiding this comment.
[P1] error-handling — Restore shared folder placements as directories
Persisting currentFolderPath widens WidgetPlacement.selectedPath to directories, but the public-share consumer still constructs ExternalSelection with type: "file" for every saved path (PublicProjectsWidget). A shared chat captured while the widget shows a folder will therefore call the share /file endpoint for that directory instead of selecting it, so the frozen folder view is not restored. Resolve the shared path through the token-scoped /resolve endpoint (as this widget does) or persist the selection kind alongside the path, and add a folder-share restore test.
zoolsher
left a comment
There was a problem hiding this comment.
Code Review: ✅ APPROVE
This PR adds an "Open in new tab" anchor to desktop widget card headers and widens the Projects widget's persisted selection from file-only to file-or-folder. The core design is sound: buildFullAppPath is a verbatim extraction of the former inline iframe-src construction and is now shared by both the iframe and the anchor, so the two addresses genuinely cannot drift. getWidgetFullHref is pure and covered by the new widget-links.test.ts. The currentFolderPath signal is derived as filesPaneDrillPath ?? selectedFolderPath and threaded consistently through the FileBrowserPage → Shell → useSelectionChangeBroadcast chain; I confirmed FilesPane.setPath writes filesPaneDrillPath on drill, so folder navigation persists on desktop as claimed. Security is fine (internal encoded paths, rel="noopener noreferrer", cross-origin/about:blank guarded in resolveLink), and the close button correctly gains its aria-label.
Findings are limited to minor code-quality nits around redundant effect churn; none block merge.
Verdict: APPROVE — Clean, well-tested refactor with a shared path builder and no correctness, security, or data-loss regressions introduced by the change.
1 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P3 | code-quality | packages/web/src/pages/free/ProjectsWidget.tsx |
Folder drilling now redundantly re-runs the workspace-context builtin effect |
| currentFolderPath: string | null; | ||
| selectedTreePaths: string[]; | ||
| }) => { | ||
| setBrowserSelection(sel); |
There was a problem hiding this comment.
[P3] code-quality — Folder drilling now redundantly re-runs the workspace-context builtin effect
handleSelectionChange now stores the whole sel (including the new currentFolderPath) via setBrowserSelection, and browserSelection is a dependency of the registry.setBuiltin(...) effect below (line 163). Because every folder drill emits a fresh broadcast with a new object identity — even when selectedPath and selectedTreePaths are unchanged — the builtin snapshot is now rebuilt and re-published on each drill with identical files. It's harmless but wasteful. Consider only storing the fields the builtin actually consumes (e.g. setBrowserSelection({ selectedPath: sel.selectedPath, selectedTreePaths: sel.selectedTreePaths })) so the folder signal drives persistence without churning the context registry.
What this PR does
A side widget lives only in the strip next to the conversation, with no way to promote it to a full screen, even though every widget already has a full-page twin:
/projects,/desktop, and/full/apps/<appId>. The Projects widget also persisted only file selections. Browsing into a folder in its list/grid pane wrote nothing back onto the placement, so the folder was lost on reload and could not ride a link.This PR adds an "Open in new tab" anchor to each widget card header, next to the close button. It is a real link, so middle-click, Cmd-click, and copy-link behave natively. The link carries the widget's current state:
/projects/<path>for the open file, or for the folder the pane is showing./full/apps/<appId>/<route>?<params>. App tiles freeze their iframesrcat mount, so the anchor re-reads the live iframe URL on hover, press, and focus through the existingresolveLink./desktop.The close button also gains the
aria-labelits mobile counterpart already has.Design & Invariants
The strip card and the full page are two views of one address. "Open in new tab" is a plain link derived from the placement, not a state handoff.
buildFullAppPathis shared by the iframesrcand the anchor, so the two constructions cannot drift.The selection broadcast reports
currentFolderPath: the folder whose contents the browser is showing. Pane drilling movesui.filesPaneDrillPathwithout touching the selection (the mirror runs the other way), so the drill path is the fresher signal and the explicit folder selection is the fallback. The placement's persistedselectedPathwidens from "file" to "file or folder". The restore path already resolves both kinds through/resolve, so folder selections now survive reload too.Interaction surfaces stay free of generic affordances: the anchor is gated with the close button on
!interaction.Alternatives considered: a per-widget overflow menu (two clicks for a single action, and the header holds only one action today), a clickable title (collides with the header being the drag activator), and an in-place maximize (not a new tab, and it loses chatting alongside the widget).
Test plan
pnpm --filter rome-web typecheckpnpm --filter rome-web test(161 files, 1326 tests, including the newwidget-links.test.ts)target="_blank", the href follows folder drilling (/projects/demo-app) and file selection (/projects/demo-app/README.md), the folder path persists onto the placement, the new tab lands on the preserved folder URL, and an app widget links to/full/apps/recipe-box.Not in this PR
chatSessionIdandinteractionride postMessage from the workspace, so an app opened full page starts without them, matching the existing "Open full view" menu action.🤖 Generated with Claude Code