feat(frontend): open the portal UI by default - #2358
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 69ba7ec6845ecaacc5a76e788c671895b267b5ea and 2ed319b. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe portal now renders by default. Only an explicit ChangesPortal enablement defaults
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/frontend/src/lib/portal/portal-store.test.ts (1)
35-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend coverage to the hook and opt-out branch.
This test verifies
readPortalEnabled()for an absent key. It does not verifyusePortalEnabled()after initialization, the server snapshot changed atsrc/frontend/src/lib/portal/portal-store.tsLine 115, or the explicit"false"branch. BecausebeforeEachsets the module state tofalsebefore clearing storage, a hook assertion here would not test default initialization. Add isolated tests for these paths, or verify that existing tests provide them and that the changed lines reach at least 80% coverage.As per coding guidelines, new and changed lines in
src/frontend/**/*.{ts,tsx}must achieve at least 80% test coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/lib/portal/portal-store.test.ts` around lines 35 - 39, Extend portal-store test coverage beyond readPortalEnabled’s absent-key case: add isolated tests for usePortalEnabled after initialization with the default enabled state, the server snapshot behavior in the portal-store implementation, and the explicit "false" opt-out branch. Ensure setup does not leave module state false when testing default initialization, and verify changed frontend lines reach at least 80% coverage.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/frontend/src/lib/portal/portal-store.test.ts`:
- Around line 35-39: Extend portal-store test coverage beyond
readPortalEnabled’s absent-key case: add isolated tests for usePortalEnabled
after initialization with the default enabled state, the server snapshot
behavior in the portal-store implementation, and the explicit "false" opt-out
branch. Ensure setup does not leave module state false when testing default
initialization, and verify changed frontend lines reach at least 80% coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b292b8fa-ee68-40f1-9664-ea1bca745204
📥 Commits
Reviewing files that changed from the base of the PR and between 0fb67d3 and 1f6da06f2e252b9b8b5ec1731842a625a91be933.
📒 Files selected for processing (3)
src/frontend/src/components/portal/portal-layout.tsxsrc/frontend/src/lib/portal/portal-store.test.tssrc/frontend/src/lib/portal/portal-store.ts
f359d02 to
69ba7ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/src/routes/index.tsx`:
- Around line 21-22: Update the comment near IndexRoute to remove the incorrect
claim that enabling the portal while mounted requires a reload. Explain that
usePortalEnabled reacts to changes and redirects immediately, while beforeLoad
serves as the initial-navigation fallback.
In `@src/frontend/src/routes/portal.tsx`:
- Around line 30-33: Update the comment above PortalRoute to accurately describe
both mount paths: when usePortalEnabled() is false, redirect the user to the
available app, and when it is true—including direct /portal navigation—render
PortalLayout. Remove the inaccurate claim that this component only mounts with
the portal disabled.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 547f545f-2f28-42f6-b8e8-a246011e23da
📥 Commits
Reviewing files that changed from the base of the PR and between 1f6da06f2e252b9b8b5ec1731842a625a91be933 and 69ba7ec6845ecaacc5a76e788c671895b267b5ea.
📒 Files selected for processing (4)
src/frontend/src/components/sidebar-settings.tsxsrc/frontend/src/lib/portal/portal-store.tssrc/frontend/src/routes/index.tsxsrc/frontend/src/routes/portal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/frontend/src/lib/portal/portal-store.ts
| // `beforeLoad` only runs on navigation, so a reader who turns the portal on | ||
| // while standing here would sit on the dashboard until a reload. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'setPortalEnabled|usePortalEnabled|subscribe' \
src/frontend/src/lib/portal/portal-store.ts \
src/frontend/src/routes/index.tsxRepository: constructorfabric/insight
Length of output: 6380
Correct the reload claim in the route comment.
IndexRoute subscribes through usePortalEnabled() via useSyncExternalStore(subscribe, ...), and setPortalEnabled() calls emit() before reading the store. If a reader enables the portal while this route is mounted, the hook re-renders and the route returns <Navigate to="/portal" replace /> without requiring a reload. Update the comment to describe beforeLoad as the initial-navigation fallback.
Proposed comment
- // `beforeLoad` only runs on navigation, so a reader who turns the portal on
- // while standing here would sit on the dashboard until a reload.
+ // `beforeLoad` handles the initial navigation. The subscribed store hook
+ // below redirects if the reader enables the portal while this route is open.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // `beforeLoad` only runs on navigation, so a reader who turns the portal on | |
| // while standing here would sit on the dashboard until a reload. | |
| // `beforeLoad` handles the initial navigation. The subscribed store hook | |
| // below redirects if the reader enables the portal while this route is open. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/frontend/src/routes/index.tsx` around lines 21 - 22, Update the comment
near IndexRoute to remove the incorrect claim that enabling the portal while
mounted requires a reload. Explain that usePortalEnabled reacts to changes and
redirects immediately, while beforeLoad serves as the initial-navigation
fallback.
| // The root shell swaps in PortalLayout for this route while the portal is | ||
| // on, so this component only mounts with it OFF — a pasted /portal URL, or a | ||
| // viewer who turned the preview off while standing here. Either way the | ||
| // portal must not render: send them to the app they do have. | ||
| // viewer who opted out while standing here. Either way the portal must not | ||
| // render: send them to the app they do have. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the mount-state description.
The comment says PortalRoute only mounts while the portal is off. Line 35 returns <PortalLayout /> when usePortalEnabled() is true, including for direct /portal navigation. Describe the disabled redirect and enabled render paths separately.
Proposed comment
- // The root shell swaps in PortalLayout for this route while the portal is
- // on, so this component only mounts with it OFF — a pasted /portal URL, or a
- // viewer who opted out while standing here. Either way the portal must not
- // render: send them to the app they do have.
+ // When the portal is disabled, a direct /portal URL or an opt-out while
+ // already here redirects to /. When enabled, render PortalLayout.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // The root shell swaps in PortalLayout for this route while the portal is | |
| // on, so this component only mounts with it OFF — a pasted /portal URL, or a | |
| // viewer who turned the preview off while standing here. Either way the | |
| // portal must not render: send them to the app they do have. | |
| // viewer who opted out while standing here. Either way the portal must not | |
| // render: send them to the app they do have. | |
| // When the portal is disabled, a direct /portal URL or an opt-out while | |
| // already here redirects to /. When enabled, render PortalLayout. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/frontend/src/routes/portal.tsx` around lines 30 - 33, Update the comment
above PortalRoute to accurately describe both mount paths: when
usePortalEnabled() is false, redirect the user to the available app, and when it
is true—including direct /portal navigation—render PortalLayout. Remove the
inaccurate claim that this component only mounts with the portal disabled.
2b0165d to
62c8622
Compare
The `insight.portal` preference now reads as opt-out: an absent key means the portal renders, and only an explicit "false" returns the legacy shell. The sidebar toggle, reachable from the portal's own footer, still switches back per reader. Both preferences share that rule now, so one `readBoolPref` states it once, and the toggle label and the two route guards drop the "preview" wording the flag no longer carries. Signed-off-by: hello1101n <hello1101n@users.noreply.github.com>
62c8622 to
2ed319b
Compare
38f9511
3 participants
What
The portal UI is now what a reader gets on open.
insight.portalflips from opt-in to opt-out: an absent key means the portal renders, and only an explicit"false"returns the legacy shell.readEnabled()→readKey(ENABLED_KEY) !== "false", mirroring theshowPlannedreaderusePortalEnabledserver snapshot →true, so the first paint matches the clientportal-store.ts/portal-layout.tsxdescribe the flag as opt-outTurning it off
Unchanged and still reachable from inside the portal:
SidebarSettingsrenders inAppSidebarFooter, which the portal's lens rail and context pane both mount. The toggle writes"false";/portalthen redirects to/and the legacy dashboard renders.showPlannedis untouched — still ON by default, still toggled from the same panel.Tests
localStoragereads as "no key"pnpm test(929 unit tests),tsc -b, eslint on the touched files — all cleanSummary by CodeRabbit
New Features
"false".Documentation
Tests