Feature/guided tour overlay - #2473
Open
Ryosuke-Asano wants to merge 14 commits into
Open
Ryosuke-Asano wants to merge 14 commits into
Ryosuke-Asano wants to merge 14 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Chrome guided-tour system (types, controller, overlay, positioning, styles, tests), Split View settings/service/UI and routing, a New Tab TipBanner with settings toggle, welcome/settings UI and locale updates, and an interactive tutorial plan. ChangesChrome Guided Tour Runtime
Split View Settings System
New Tab Tip Banner System
Welcome Features Page Expansion & LearnButton
Implementation Plan Document
Sequence Diagram(s) sequenceDiagram
participant LearnButton
participant PrefStore
participant TourController
participant TourOverlay
participant WorkspacePanelGuard
LearnButton->>PrefStore: set floorp.guidedTour.request({tourId})
PrefStore->>TourController: pref observer triggers start(tourId)
TourController->>WorkspacePanelGuard: ensureOpen() (when needed)
TourController->>TourOverlay: update state (active, step, targetRect)
TourOverlay->>TourController: user actions (next/prev/stop)
Estimated code review effort: Possibly related PRs:
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a preference-driven “Guided Tour” overlay in browser chrome and wires it up to UI entry points in Floorp Hub (settings pages). It also expands “feature discovery” by adding a Split View settings page, a New Tab “Tip” banner, and updated welcome/feature content + i18n strings.
Changes:
- Add a browser-chrome Guided Tour system (tour definitions + overlay UI) driven by a single pref (
floorp.guidedTour.active). - Add “Learn how to use” entry points in Hub feature pages and introduce a new Split View settings page.
- Add feature-discovery UI/strings (New Tab tip banner, welcome Split View card, guided tour locale strings).
Reviewed changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| nimbalyst-local/plans/1-6-twinkly-thompson.md | Adds an implementation plan document for interactive/tutorial work. |
| i18n/ja-JP/browser-chrome.json | Adds guided tour strings (JA) for chrome-level overlay. |
| i18n/en-US/browser-chrome.json | Adds guided tour strings (EN) for chrome-level overlay. |
| browser-features/pages-welcome/src/lib/i18n/locales/ja-JP.json | Reformat + adds Split View strings (JA) for welcome/features content. |
| browser-features/pages-welcome/src/lib/i18n/locales/en-US.json | Reformat + adds Split View strings (EN) for welcome/features content. |
| browser-features/pages-welcome/src/app/features/page.tsx | Adds Split View feature card and animated preview; updates gesture preview rendering. |
| browser-features/pages-welcome/src/app/features/assets/splitview.svg | Adds Split View illustration asset. |
| browser-features/pages-settings/src/types/pref.ts | Introduces SplitViewLayout + SplitViewFormData types for settings forms. |
| browser-features/pages-settings/src/lib/i18n/locales/en-US.json | Reformat + adds Split View strings and help/description copy. |
| browser-features/pages-settings/src/components/nav-features.tsx | Adds optional badge support for internal nav items. |
| browser-features/pages-settings/src/components/common/learn-button.tsx | Adds a reusable button that starts a guided tour via pref write. |
| browser-features/pages-settings/src/components/app-sidebar.tsx | Adds Split View to Hub sidebar and shows a “New” badge. |
| browser-features/pages-settings/src/app/workspaces/page.tsx | Adds “Learn” button to start the Workspaces tour. |
| browser-features/pages-settings/src/app/splitview/page.tsx | New Hub page for Split View settings + “Learn” button. |
| browser-features/pages-settings/src/app/splitview/dataManager.ts | New RPC-backed read/write layer for Split View prefs. |
| browser-features/pages-settings/src/app/splitview/components/LayoutSettings.tsx | New layout picker UI for Split View layouts. |
| browser-features/pages-settings/src/app/splitview/components/BasicSettings.tsx | New basic settings UI for Split View enable/max panes + docs link. |
| browser-features/pages-settings/src/app/sidebar/page.tsx | Minor layout tweak (spacing/empty wrapper removal). |
| browser-features/pages-settings/src/app/gesture/page.tsx | Adds “Learn” button to start the Mouse Gestures tour. |
| browser-features/pages-settings/src/app/design/page.tsx | Minor layout tweak (spacing). |
| browser-features/pages-settings/src/app/design/components/Interface.tsx | Adds per-theme descriptive text in the theme selector. |
| browser-features/pages-settings/src/App.tsx | Registers the new Split View route in Hub. |
| browser-features/pages-newtab/src/lib/i18n/locales/ja-JP.json | Adds TipBanner strings (JA) and new setting label. |
| browser-features/pages-newtab/src/lib/i18n/locales/en-US.json | Adds TipBanner strings (EN) and new setting label. |
| browser-features/pages-newtab/src/contexts/ComponentsContext.tsx | Adds tipBanner component toggle to New Tab layout state. |
| browser-features/pages-newtab/src/components/TipBanner/index.tsx | New TipBanner component: rotates “feature tips” and links to Hub pages; tracks seen tips. |
| browser-features/pages-newtab/src/components/Settings/index.tsx | Adds a checkbox to enable/disable TipBanner. |
| browser-features/pages-newtab/src/App.tsx | Renders TipBanner when enabled by components settings. |
| browser-features/chrome/common/guided-tour/types.ts | Defines tour model types and the pref name used to control tours. |
| browser-features/chrome/common/guided-tour/tour-definitions/workspaces.ts | Adds Workspaces tour step definitions. |
| browser-features/chrome/common/guided-tour/tour-definitions/split-view.ts | Adds Split View tour step definitions. |
| browser-features/chrome/common/guided-tour/tour-definitions/mouse-gestures.ts | Adds Mouse Gestures tour step definitions. |
| browser-features/chrome/common/guided-tour/tour-definitions/index.ts | Adds a registry/lookup for tours by id. |
| browser-features/chrome/common/guided-tour/overlay.ts | Implements the actual overlay DOM/CSS + spotlight + tooltip positioning. |
| browser-features/chrome/common/guided-tour/index.ts | Observes the tour pref, translates step text, drives overlay lifecycle and step navigation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (8)
nimbalyst-local/plans/1-6-twinkly-thompson.md (2)
278-278: ⚡ Quick winFix heading level jump.
The heading at line 278 jumps from H1 (
# === 以下: 旧計画（実装済み） ===) to H3 (### 1-1. Split View 設定ページを新規作成). Markdown best practices require incrementing heading levels by one step at a time. Change the H3 headings under the "旧計画" section to H2 (##) to maintain proper document structure.🤖 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 `@nimbalyst-local/plans/1-6-twinkly-thompson.md` at line 278, The H3 headings under the "=== 以下: 旧計画（実装済み） ===" section (for example the line with "### 1-1. Split View 設定ページを新規作成") jump one level down from the H1; change those H3 headings to H2 (replace "###" with "##") so the heading levels increment by only one step and maintain proper document structure.Source: Linters/SAST tools
76-98: 💤 Low valueAdd language identifiers to code blocks.
The tutorial step outlines at lines 76-98, 140-157, and 188-200 are enclosed in triple backticks without language specifiers. While these are pseudo-code outlines rather than executable code, adding a language identifier (e.g.,
text ormarkdown) would improve readability and silence linter warnings.Also applies to: 140-157, 188-200
🤖 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 `@nimbalyst-local/plans/1-6-twinkly-thompson.md` around lines 76 - 98, The fenced code blocks containing the step outlines (the blocks that start with Step 1 / Step 2 / Step 3 / Step 4 / Step 5) are using plain triple backticks with no language; update those fences to include a language identifier (for example ```text or ```markdown) so linters/renderers treat them consistently and readability improves—locate the blocks that include the "Step 1: (text) 「マウスジェスチャーとは」" through "Step 5: (text) 「カスタマイズ」" and change the opening backticks accordingly (same fix for the other similar blocks mentioned).Source: Linters/SAST tools
browser-features/pages-newtab/src/components/TipBanner/index.tsx (4)
49-56: ⚡ Quick winAdd error logging with feature prefix.
Per coding guidelines, error handling should include
console.errorwith a feature prefix. Currently, the catch block at line 53 silently returns an empty array without logging.📋 Suggested improvement
async function getSeenTips(): Promise<string[]> { try { const raw = await rpc.getStringPref(PREF_SEEN_TIPS); return raw ? JSON.parse(raw) : []; } catch (e) { + console.error('[TipBanner] Failed to load seen tips:', e); return []; } }🤖 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 `@browser-features/pages-newtab/src/components/TipBanner/index.tsx` around lines 49 - 56, In getSeenTips, the catch silently swallows errors when reading PREF_SEEN_TIPS; update the catch to call console.error with a feature prefix (e.g. "[NewTab][TipBanner]") and include the caught error (and optionally the raw value) so failures are logged for debugging; keep the function returning [] after logging.Source: Coding guidelines
6-11: 💤 Low valueMove type definition to separate types.ts file.
As per coding guidelines, type definitions should be separated into dedicated
types.tsfiles (except for.sys.mtsfiles). TheTipinterface should be extracted to a separate types file.🤖 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 `@browser-features/pages-newtab/src/components/TipBanner/index.tsx` around lines 6 - 11, Extract the Tip interface from index.tsx into a new types.ts file and export it, then import and use that type in the TipBanner component; specifically, create types.ts exporting interface Tip { id: string; titleKey: string; descriptionKey: string; hubUrl: string; }, remove the local Tip declaration in pages-newtab/src/components/TipBanner/index.tsx, add an import { Tip } from './types' (or appropriate relative path) and ensure any usages of Tip within TipBanner (props, state, helpers) reference the imported type.Source: Coding guidelines
89-102: ⚡ Quick winAdd error logging to tip selection effect.
The
pickTipasync function inside useEffect has no error handling. IfgetSeenTipsfails (already handled) or state updates fail, errors should be logged with the feature prefix per guidelines.📋 Suggested improvement
useEffect(() => { if (getSessionDismissed()) return; const pickTip = async () => { - const seen = await getSeenTips(); - const unseen = tips.filter((tip) => !seen.includes(tip.id)); - const pool = unseen.length > 0 ? unseen : tips; - const tip = pool[Math.floor(Math.random() * pool.length)]; - setCurrentTip(tip); - requestAnimationFrame(() => setMounted(true)); + try { + const seen = await getSeenTips(); + const unseen = tips.filter((tip) => !seen.includes(tip.id)); + const pool = unseen.length > 0 ? unseen : tips; + const tip = pool[Math.floor(Math.random() * pool.length)]; + setCurrentTip(tip); + requestAnimationFrame(() => setMounted(true)); + } catch (e) { + console.error('[TipBanner] Failed to pick tip:', e); + } }; pickTip(); }, []);🤖 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 `@browser-features/pages-newtab/src/components/TipBanner/index.tsx` around lines 89 - 102, The useEffect's async helper pickTip lacks error handling; wrap its body in try/catch and log any errors with the feature prefix (use the project's logger/processLogger) so failures from getSeenTips or state updates are recorded. Specifically, update the pickTip function (inside useEffect) to call getSeenTips and the tip selection logic inside a try block, and in the catch block call the logger (including the feature prefix) with the error; keep setCurrentTip and requestAnimationFrame(() => setMounted(true)) in the try block so they only run on success.Source: Coding guidelines
49-80: 💤 Low valueAdd JSDoc comments to helper functions.
Per coding guidelines, functions should have JSDoc comments explaining their purpose and parameters. Consider adding documentation for
getSeenTips,markTipSeen,getSessionDismissed, andsetSessionDismissed.🤖 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 `@browser-features/pages-newtab/src/components/TipBanner/index.tsx` around lines 49 - 80, Add JSDoc comments above each helper function to describe purpose, parameters and return types: document getSeenTips(): Promise<string[]> (explaining it reads PREF_SEEN_TIPS and returns parsed array or [] on error), markTipSeen(id: string): Promise<void> (explain it marks a tip id as seen and persists to PREF_SEEN_TIPS), getSessionDismissed(): boolean (explain it reads PREF_DISMISSED_SESSION from sessionStorage and returns boolean, default false on error), and setSessionDismissed(): void (explain it sets PREF_DISMISSED_SESSION in sessionStorage and silently ignores storage errors); use /** ... */ style JSDoc and include `@param` and `@returns` tags where applicable to satisfy the coding guidelines.Source: Coding guidelines
browser-features/pages-settings/src/app/splitview/dataManager.ts (1)
1-2: ⚡ Quick winUse the configured path alias for
rpcimport.This file mixes relative and alias imports; using the project alias keeps pathing consistent and resilient to folder moves.
Suggested change
-import { rpc } from "../../lib/rpc/rpc.ts"; +import { rpc } from "`@/lib/rpc/rpc.ts`";As per coding guidelines, "Use path aliases defined in deno.json for imports:
#i18n/,#chrome/,#libs/,#features-chrome/,#modules/,#ui/,#themes/".🤖 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 `@browser-features/pages-settings/src/app/splitview/dataManager.ts` around lines 1 - 2, The import for rpc should use the project's configured path alias instead of a relative path; update the import statement that currently references "../../lib/rpc/rpc.ts" to the appropriate alias (e.g., "`#libs/rpc/rpc.ts`" or the project's rpc alias) so code in dataManager.ts consistently uses path aliases; ensure the symbol rpc remains unchanged and that the new import path matches the alias defined in deno.json so types and runtime resolution continue to work.Source: Coding guidelines
browser-features/pages-settings/src/app/splitview/components/LayoutSettings.tsx (1)
12-16: ⚡ Quick winMove
LayoutOptionto atypes.tsfile and typevalueasSplitViewFormData["layout"].This removes the cast at
setValueand keeps layout options contract-safe.Suggested change
-type LayoutOption = { - value: string; +type LayoutOption = { + value: SplitViewFormData["layout"]; labelKey: string; preview: React.ReactNode; }; ... - onClick={() => setValue("layout", layout.value as SplitViewFormData["layout"])} + onClick={() => setValue("layout", layout.value)}As per coding guidelines, "Separate type definitions into dedicated
types.tsfiles, except for.sys.mtsfiles".Also applies to: 119-119
🤖 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 `@browser-features/pages-settings/src/app/splitview/components/LayoutSettings.tsx` around lines 12 - 16, Move the LayoutOption type into a dedicated types.ts and change its value property to use the form-safe union by referring to SplitViewFormData["layout"]; update imports where LayoutOption is used (e.g., in LayoutSettings.tsx) to import from the new types.ts, and remove the unnecessary cast at the call site that uses setValue so setValue(...) accepts the typed value directly; ensure SplitViewFormData is imported or re-exported so the type reference resolves.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.
Inline comments:
In `@browser-features/chrome/common/guided-tour/index.ts`:
- Around line 75-91: The waitForSelector timeout path can advance UI without
updating the stored tour state and leaves its timer running; update the
stored/current tour state (e.g., currentTour/currentStep or the preference write
used elsewhere) when calling showStep(tourId, stepIndex + 1) from the timeout,
and make the timeout cancellable by storing its ID (from setTimeout) on the tour
instance or a closure variable so cleanup/skip routines can
clearTimeout(timerId); additionally, before invoking showStep from the retry
callback verify the tour is still active and the expected currentStep/tourId
matches to avoid operating on stale state.
- Around line 23-31: The parsePref function currently trusts raw JSON and casts
to TourState, causing missing properties (e.g., currentStep) to be undefined and
later crash in showStep; update parsePref to validate the parsed object shape
before casting—after JSON.parse(raw) check that required fields (like tourId as
string and currentStep as a finite number or step present) exist and have
correct types, and return null if validation fails; reference the parsePref
function, the TourState type, TOUR_PREF constant and showStep usage so the guard
covers the exact fields showStep expects.
In `@browser-features/chrome/common/guided-tour/overlay.ts`:
- Around line 205-207: The button labels in overlay.ts are hardcoded (e.g.,
skipBtn, nextBtn, prevBtn, finishBtn) and must use the translated labels passed
from guided-tour/index.ts; update the overlay component to accept a labels
object (or use the passed-in guidedTour.labels) and set each element's
textContent to labels.skip / labels.next / labels.previous / labels.finish with
safe fallbacks to English if a key is missing, replacing all hardcoded strings
at the locations that create/set skipBtn, nextBtn, prevBtn, and finishBtn.
- Around line 402-415: The tooltip repositioning currently always uses the
hardcoded "bottom" when calling positionTooltip in repositionTooltip; instead
read the active step's placement (e.g. this.currentStep?.placement) or fallback
to the overlay's stored placement property (e.g. this.placement) and pass that
placement into positionTooltip(rect, placement) so resize preserves the actual
step's configured side; update repositionTooltip to compute a placement variable
(currentStep placement || overlay placement || "bottom") and use that when
calling positionTooltip.
- Around line 364-399: The placement clamping currently clamps the raw left/top
then applies transforms (see placement switch, variables left/top/transform and
this.tooltipEl.style.transform), which lets translateX(-50%) or translateY(-50%)
push the tooltip off-screen; fix by clamping the post-transform visual position
instead: when transform includes "translateX(-50%)" treat left as the horizontal
center and clamp it between gap + tooltipWidth/2 and vw - tooltipWidth/2 - gap
(set this.tooltipEl.style.left to `${left}px` and keep transform), and when
transform includes "translateY(-50%)" treat top as vertical center and clamp
between gap + tooltipHeight/2 and vh - tooltipHeight/2 - gap; for placements
without those transforms keep the existing clamp logic; update the clamping
logic before assigning this.tooltipEl.style.left/top so the final transformed
tooltip cannot go off-screen.
In `@browser-features/pages-newtab/src/components/TipBanner/index.tsx`:
- Around line 58-64: The markTipSeen function lacks error handling; wrap its
body in a try-catch so any failures from getSeenTips or rpc.setStringPref are
caught and logged (use console.error) to avoid silent failures when callers use
the void operator; ensure you still return/resolve the Promise after handling
the error and include identifying context in the log (e.g., tip id and
PREF_SEEN_TIPS) so debugging rpc.setStringPref/getSeenTips problems is easier.
In
`@browser-features/pages-settings/src/app/splitview/components/BasicSettings.tsx`:
- Around line 66-69: The external link anchor in the BasicSettings component
(the <a href="https://docs.floorp.app/docs/features/split-view" target="_blank"
...> element) is missing a rel attribute; update that anchor in
BasicSettings.tsx to include rel="noopener noreferrer" so opening the external
page with target="_blank" does not expose window.opener.
In `@browser-features/pages-settings/src/app/splitview/dataManager.ts`:
- Around line 25-50: saveSplitViewSettings currently calls rpc.set* without
error handling; wrap the Promise.all RPC write call in a try/catch and on error
call console.error('[SplitView]', ...) with the caught error and a short context
message; do the same for the corresponding read path (loadSplitViewSettings /
any rpc.get* calls around lines 52-57) so both save and load RPC interactions
are protected from unhandled rejections and produce feature-prefixed logs.
- Around line 79-88: The code reads raw.flexRatios, gridColRatio and
gridRowRatio directly and can accept malformed values; update the parsing so
flexRatios is normalized to an array of finite numbers (e.g. map values with
Number(), filter Number.isFinite, optionally keep only positives) and only
assign to paneSizes.flexRatios when the resulting array length > 0, and for
gridColRatio/gridRowRatio parse numeric strings with Number() and only assign
when Number.isFinite(value) (or within expected range if required by
SplitViewFormData). Target the variables raw, paneSizes.flexRatios,
paneSizes.gridColRatio and paneSizes.gridRowRatio in dataManager.ts and ensure
the normalized/validated values are what you return/write back.
In `@browser-features/pages-settings/src/app/splitview/page.tsx`:
- Around line 20-43: The async effects for loading and saving settings lack
error handling; wrap the body of fetchDefaultValues (used in the first
React.useEffect and as the "onfocus" listener) in try/catch and log failures
with console.error("[SplitView]", err) so promise rejections are handled, and
similarly wrap the save call in the second React.useEffect (around
saveSplitViewSettings(watchAll as SplitViewFormData)) in a try/catch with the
same console.error prefix; ensure the event listener continues to call the
error-handling fetchDefaultValues wrapper so exceptions never become unhandled.
- Around line 31-35: The event listener currently uses the wrong event name
"onfocus" on document.documentElement; change both
document.documentElement.addEventListener and removeEventListener to use the
correct event type (e.g., "focus" or "focusin" as appropriate for your refresh
behavior) so fetchDefaultValues is triggered. Also make fetchDefaultValues
robust: wrap the await getSplitViewSettings() call in a try/catch and on error
call console.error("[SplitView]", error) (or include a short message plus the
error) to avoid unhandled promise rejections; update any callers that expect the
old behavior accordingly.
In `@browser-features/pages-settings/src/components/common/learn-button.tsx`:
- Around line 15-16: The payload uses the wrong key name ("step") causing the
guided-tour reader to miss it; update the serialized object passed to
rpc.setStringPref so it contains currentStep instead of step (i.e., build
payload as JSON.stringify({ tourId, currentStep: 0 })) so the guided-tour reader
consumes the correct field; ensure the change is applied where payload is
created before calling rpc.setStringPref("floorp.guidedTour.active", payload).
- Around line 14-17: The click handler handleStartTour currently awaits
rpc.setStringPref without local error handling; wrap the async call in a
try/catch inside handleStartTour (the useCallback) so any rejection is caught,
and on error call console.error with a feature-prefixed tag (for example
console.error('[GuidedTour]', error, { tourId, payload })) to include the error
and relevant context (tourId/payload); ensure you still await/set the pref
inside the try block and preserve the dependency on tourId.
In `@browser-features/pages-welcome/src/app/features/page.tsx`:
- Line 7: Remove the unused import "splitviewSvg" from the top of the module
(the import statement importing splitviewSvg from "./assets/splitview.svg")
since the component uses an inline animated SVG instead; delete that import line
and any now-unused identifier references so the module has no unused imports.
In `@i18n/en-US/browser-chrome.json`:
- Around line 462-465: The step2 description in the i18n key currently tells the
user to "Click the workspace button", but workspaces.ts defines an automatic
action (action: { type: "click", selector: "`#workspaces-toolbar-button`" }) that
already opens the panel before the tooltip appears; update the "step2"
description text to reflect that the workspace panel is already open and then
explain what the user can do (e.g., "The workspace panel is now open — use it to
switch between workspaces, create new ones, or manage existing ones."),
referencing the i18n key "step2" and the automated action in workspaces.ts.
- Around line 470-473: The description for the "step4" i18n key is inaccurate
because workspaces.ts automatically triggers action: { type: "rightClick",
selector: "`#workspaces-toolbar-button`" } before showing the step; update the
"step4" description in i18n/en-US/browser-chrome.json (key "step4" title
"Customizing Workspaces") to state that the context menu is already open (e.g.,
"The workspace context menu is open. You can rename, reorder, archive, or delete
workspaces, and assign Firefox Containers for extra privacy."), so it matches
the automated right-click behavior defined in workspaces.ts.
In `@nimbalyst-local/plans/1-6-twinkly-thompson.md`:
- Around line 3-7: The "COMPLETED ✓" status is inconsistent with the Context
that describes unimplemented interactive demos and references files like
tutorial-modal.tsx that aren’t in this PR; either mark the document as a
planned/IN-PROGRESS item (e.g., change "COMPLETED ✓" to "PLANNED" or
"INCOMPLETE") or, if this is a post-implementation record, add the missing
implementation files (including tutorial-modal.tsx) and update the content to
list the actual committed changes; also add a short note at the top clarifying
whether this file is a retrospective of completed work or a forward-looking plan
for future PRs so reviewers know which action is expected.
---
Nitpick comments:
In `@browser-features/pages-newtab/src/components/TipBanner/index.tsx`:
- Around line 49-56: In getSeenTips, the catch silently swallows errors when
reading PREF_SEEN_TIPS; update the catch to call console.error with a feature
prefix (e.g. "[NewTab][TipBanner]") and include the caught error (and optionally
the raw value) so failures are logged for debugging; keep the function returning
[] after logging.
- Around line 6-11: Extract the Tip interface from index.tsx into a new types.ts
file and export it, then import and use that type in the TipBanner component;
specifically, create types.ts exporting interface Tip { id: string; titleKey:
string; descriptionKey: string; hubUrl: string; }, remove the local Tip
declaration in pages-newtab/src/components/TipBanner/index.tsx, add an import {
Tip } from './types' (or appropriate relative path) and ensure any usages of Tip
within TipBanner (props, state, helpers) reference the imported type.
- Around line 89-102: The useEffect's async helper pickTip lacks error handling;
wrap its body in try/catch and log any errors with the feature prefix (use the
project's logger/processLogger) so failures from getSeenTips or state updates
are recorded. Specifically, update the pickTip function (inside useEffect) to
call getSeenTips and the tip selection logic inside a try block, and in the
catch block call the logger (including the feature prefix) with the error; keep
setCurrentTip and requestAnimationFrame(() => setMounted(true)) in the try block
so they only run on success.
- Around line 49-80: Add JSDoc comments above each helper function to describe
purpose, parameters and return types: document getSeenTips(): Promise<string[]>
(explaining it reads PREF_SEEN_TIPS and returns parsed array or [] on error),
markTipSeen(id: string): Promise<void> (explain it marks a tip id as seen and
persists to PREF_SEEN_TIPS), getSessionDismissed(): boolean (explain it reads
PREF_DISMISSED_SESSION from sessionStorage and returns boolean, default false on
error), and setSessionDismissed(): void (explain it sets PREF_DISMISSED_SESSION
in sessionStorage and silently ignores storage errors); use /** ... */ style
JSDoc and include `@param` and `@returns` tags where applicable to satisfy the
coding guidelines.
In
`@browser-features/pages-settings/src/app/splitview/components/LayoutSettings.tsx`:
- Around line 12-16: Move the LayoutOption type into a dedicated types.ts and
change its value property to use the form-safe union by referring to
SplitViewFormData["layout"]; update imports where LayoutOption is used (e.g., in
LayoutSettings.tsx) to import from the new types.ts, and remove the unnecessary
cast at the call site that uses setValue so setValue(...) accepts the typed
value directly; ensure SplitViewFormData is imported or re-exported so the type
reference resolves.
In `@browser-features/pages-settings/src/app/splitview/dataManager.ts`:
- Around line 1-2: The import for rpc should use the project's configured path
alias instead of a relative path; update the import statement that currently
references "../../lib/rpc/rpc.ts" to the appropriate alias (e.g.,
"`#libs/rpc/rpc.ts`" or the project's rpc alias) so code in dataManager.ts
consistently uses path aliases; ensure the symbol rpc remains unchanged and that
the new import path matches the alias defined in deno.json so types and runtime
resolution continue to work.
In `@nimbalyst-local/plans/1-6-twinkly-thompson.md`:
- Line 278: The H3 headings under the "=== 以下: 旧計画（実装済み） ===" section (for
example the line with "### 1-1. Split View 設定ページを新規作成") jump one level down from
the H1; change those H3 headings to H2 (replace "###" with "##") so the heading
levels increment by only one step and maintain proper document structure.
- Around line 76-98: The fenced code blocks containing the step outlines (the
blocks that start with Step 1 / Step 2 / Step 3 / Step 4 / Step 5) are using
plain triple backticks with no language; update those fences to include a
language identifier (for example ```text or ```markdown) so linters/renderers
treat them consistently and readability improves—locate the blocks that include
the "Step 1: (text) 「マウスジェスチャーとは」" through "Step 5: (text) 「カスタマイズ」" and change
the opening backticks accordingly (same fix for the other similar blocks
mentioned).
🪄 Autofix (Beta)
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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 46c765f2-aed0-4cc0-a36c-07abd1868cfa
⛔ Files ignored due to path filters (1)
browser-features/pages-welcome/src/app/features/assets/splitview.svgis excluded by!**/*.svg
📒 Files selected for processing (35)
browser-features/chrome/common/guided-tour/index.tsbrowser-features/chrome/common/guided-tour/overlay.tsbrowser-features/chrome/common/guided-tour/tour-definitions/index.tsbrowser-features/chrome/common/guided-tour/tour-definitions/mouse-gestures.tsbrowser-features/chrome/common/guided-tour/tour-definitions/split-view.tsbrowser-features/chrome/common/guided-tour/tour-definitions/workspaces.tsbrowser-features/chrome/common/guided-tour/types.tsbrowser-features/pages-newtab/src/App.tsxbrowser-features/pages-newtab/src/components/Settings/index.tsxbrowser-features/pages-newtab/src/components/TipBanner/index.tsxbrowser-features/pages-newtab/src/contexts/ComponentsContext.tsxbrowser-features/pages-newtab/src/lib/i18n/locales/en-US.jsonbrowser-features/pages-newtab/src/lib/i18n/locales/ja-JP.jsonbrowser-features/pages-settings/src/App.tsxbrowser-features/pages-settings/src/app/design/components/Interface.tsxbrowser-features/pages-settings/src/app/design/page.tsxbrowser-features/pages-settings/src/app/gesture/page.tsxbrowser-features/pages-settings/src/app/sidebar/page.tsxbrowser-features/pages-settings/src/app/splitview/components/BasicSettings.tsxbrowser-features/pages-settings/src/app/splitview/components/LayoutSettings.tsxbrowser-features/pages-settings/src/app/splitview/dataManager.tsbrowser-features/pages-settings/src/app/splitview/page.tsxbrowser-features/pages-settings/src/app/workspaces/page.tsxbrowser-features/pages-settings/src/components/app-sidebar.tsxbrowser-features/pages-settings/src/components/common/learn-button.tsxbrowser-features/pages-settings/src/components/nav-features.tsxbrowser-features/pages-settings/src/lib/i18n/locales/en-US.jsonbrowser-features/pages-settings/src/lib/i18n/locales/ja-JP.jsonbrowser-features/pages-settings/src/types/pref.tsbrowser-features/pages-welcome/src/app/features/page.tsxbrowser-features/pages-welcome/src/lib/i18n/locales/en-US.jsonbrowser-features/pages-welcome/src/lib/i18n/locales/ja-JP.jsoni18n/en-US/browser-chrome.jsoni18n/ja-JP/browser-chrome.jsonnimbalyst-local/plans/1-6-twinkly-thompson.md
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
browser-features/pages-settings/src/components/common/learn-button.tsx (1)
14-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap the async pref write in try/catch with feature-prefixed logging.
The click handler awaits an RPC call without local error handling; failures can become unhandled rejections and give no actionable diagnostics. As per coding guidelines, error handling with try/catch and
console.error('[FeatureName]', ...)is required.🛡️ Proposed fix
const handleStartTour = useCallback(async () => { - const payload = JSON.stringify({ tourId, currentStep: 0 }); - await rpc.setStringPref("floorp.guidedTour.active", payload); + try { + const payload = JSON.stringify({ tourId, currentStep: 0 }); + await rpc.setStringPref("floorp.guidedTour.active", payload); + } catch (error) { + console.error("[GuidedTour]", error, { tourId }); + } }, [tourId]);🤖 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 `@browser-features/pages-settings/src/components/common/learn-button.tsx` around lines 14 - 17, The async click handler handleStartTour currently awaits rpc.setStringPref without local error handling; wrap the await call in a try/catch inside handleStartTour, keep building payload as before (JSON.stringify({ tourId, currentStep: 0 })), and on error call console.error with a feature-prefixed tag like console.error('[LearnButton]', err) to surface diagnostics for rpc.setStringPref failures.Source: Coding guidelines
🧹 Nitpick comments (3)
browser-features/chrome/common/guided-tour/overlay.ts (2)
59-121: 💤 Low valueClean up injected styles on destroy.
The injected
<style>element is not removed indestroy(). Whilecreate()removes any existing style before injecting (line 61), the final cleanup when the overlay is destroyed permanently leaves the style element orphaned.♻️ Suggested fix
Store a reference to the style element and remove it in
destroy():export class GuidedTourOverlay { private spotlightPanel: XulPanel | null = null; private spotlightBox: HTMLDivElement | null = null; private tooltipPanel: XulPanel | null = null; private tooltipEl: HTMLDivElement | null = null; + private styleEl: HTMLStyleElement | null = null; ... create(): void { const doc = this.targetWindow.document; if (!doc) return; - injectStyles(doc); + this.styleEl = injectStyles(doc); ... } destroy(): void { ... + this.styleEl?.remove(); + this.styleEl = null; } }And update
injectStylesto return the element:-function injectStyles(doc: Document): void { +function injectStyles(doc: Document): HTMLStyleElement { const existing = doc.getElementById(STYLES_ID); if (existing) existing.remove(); const style = doc.createElement("style"); ... doc.head!.appendChild(style); + return style; }🤖 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 `@browser-features/chrome/common/guided-tour/overlay.ts` around lines 59 - 121, The injected style block created by injectStyles is never removed on overlay teardown; modify injectStyles to return the created HTMLStyleElement (still removing any existing element by STYLES_ID as before), update create() to store that returned element on the Overlay instance (e.g., this.styleEl), and then remove this.styleEl in destroy() (or null-check and remove by id if needed) so the injected styles are cleaned up when the overlay is destroyed.
214-220: 💤 Low valueConsider gating verbose console logging behind a debug flag.
The overlay includes extensive console logging across positioning, state checks, and panel management. While helpful for development, this may be too verbose for production.
💡 Example pattern
const DEBUG_GUIDED_TOUR = false; // or read from pref if (DEBUG_GUIDED_TOUR) { console.log("[GuidedTour:updateStep]", { ... }); }Also applies to: 260-265, 296-299, 301-305, 324-330, 332-335, 341-344, 379-383, 404-410, 413-413
🤖 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 `@browser-features/chrome/common/guided-tour/overlay.ts` around lines 214 - 220, Introduce a single debug flag (e.g., DEBUG_GUIDED_TOUR or read from a prefs/config) in overlay.ts and wrap every console.log in the Guided Tour codebase behind that flag—specifically the logs inside updateStep, positioning/state checks, and panel management (the console statements currently around the updateStep block and the other logging sites noted in the review). Replace direct console.log calls with conditional logging so that when DEBUG_GUIDED_TOUR is false these logs are not emitted; ensure the flag can be toggled for development (and is false/stripped in production) and apply the same change consistently for all console.log occurrences in this module.browser-features/chrome/common/guided-tour/tour-definitions/workspaces.ts (1)
17-24: ⚡ Quick winConsider adding a
waitTimeoutfor defensive synchronization.If the panel fails to open (e.g., due to timing issues or browser state), the tour will hang indefinitely waiting for
#workspacesToolbarButtonPanel. Adding a timeout provides a graceful degradation path.🛡️ Suggested addition
{ selector: "`#workspacesToolbarButtonPanel`", titleKey: "guidedTour.workspaces.step2.title", descriptionKey: "guidedTour.workspaces.step2.description", tooltipPlacement: "bottom", action: { type: "click", selector: "`#workspaces-toolbar-button`" }, waitForSelector: "`#workspacesToolbarButtonPanel`", + waitTimeout: 5000, },🤖 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 `@browser-features/chrome/common/guided-tour/tour-definitions/workspaces.ts` around lines 17 - 24, Add a defensive waitTimeout to the guided tour step that waits for "`#workspacesToolbarButtonPanel`" so the tour won't hang indefinitely; update the step object (the entry containing selector "`#workspacesToolbarButtonPanel`" and action.selector "`#workspaces-toolbar-button`") to include a waitTimeout (e.g., 5000–10000 ms) and ensure the tour logic will treat the timeout as a graceful failure path (skip or log) rather than blocking.
🤖 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 `@browser-features/chrome/common/guided-tour/overlay.ts`:
- Around line 396-398: The overlay currently hardcodes a tour-specific DOM id
("workspacesToolbarButtonPanel") via wsPanel/wsPanelRect/wsPanelOpen, coupling
the generic overlay to a single tour; replace this by either removing the panel
check entirely or making it generic: change the lookup that assigns wsPanel to
query by a shared class or data-attribute (e.g. querySelectorAll for a class or
[data-tour-panel]) and compute the rect and open state from that result, or
eliminate the wsPanel/wsPanelRect/wsPanelOpen logic and rely solely on existing
tooltip/spotlight state; update any code paths that reference wsPanelOpen
accordingly to use the new generic selector or removed-check behavior.
- Around line 222-223: Replace direct assignment of i18n keys to DOM with their
localized strings: instead of setting this.tooltipTitleEl.textContent =
step.titleKey and this.tooltipDescEl.textContent = step.descriptionKey, call the
translation API (e.g., chrome.i18n.getMessage or the project i18n.translate/t
function) with step.titleKey and step.descriptionKey and assign those returned
localized strings to this.tooltipTitleEl.textContent and
this.tooltipDescEl.textContent, falling back to the original key if translation
returns empty.
---
Duplicate comments:
In `@browser-features/pages-settings/src/components/common/learn-button.tsx`:
- Around line 14-17: The async click handler handleStartTour currently awaits
rpc.setStringPref without local error handling; wrap the await call in a
try/catch inside handleStartTour, keep building payload as before
(JSON.stringify({ tourId, currentStep: 0 })), and on error call console.error
with a feature-prefixed tag like console.error('[LearnButton]', err) to surface
diagnostics for rpc.setStringPref failures.
---
Nitpick comments:
In `@browser-features/chrome/common/guided-tour/overlay.ts`:
- Around line 59-121: The injected style block created by injectStyles is never
removed on overlay teardown; modify injectStyles to return the created
HTMLStyleElement (still removing any existing element by STYLES_ID as before),
update create() to store that returned element on the Overlay instance (e.g.,
this.styleEl), and then remove this.styleEl in destroy() (or null-check and
remove by id if needed) so the injected styles are cleaned up when the overlay
is destroyed.
- Around line 214-220: Introduce a single debug flag (e.g., DEBUG_GUIDED_TOUR or
read from a prefs/config) in overlay.ts and wrap every console.log in the Guided
Tour codebase behind that flag—specifically the logs inside updateStep,
positioning/state checks, and panel management (the console statements currently
around the updateStep block and the other logging sites noted in the review).
Replace direct console.log calls with conditional logging so that when
DEBUG_GUIDED_TOUR is false these logs are not emitted; ensure the flag can be
toggled for development (and is false/stripped in production) and apply the same
change consistently for all console.log occurrences in this module.
In `@browser-features/chrome/common/guided-tour/tour-definitions/workspaces.ts`:
- Around line 17-24: Add a defensive waitTimeout to the guided tour step that
waits for "`#workspacesToolbarButtonPanel`" so the tour won't hang indefinitely;
update the step object (the entry containing selector
"`#workspacesToolbarButtonPanel`" and action.selector
"`#workspaces-toolbar-button`") to include a waitTimeout (e.g., 5000–10000 ms) and
ensure the tour logic will treat the timeout as a graceful failure path (skip or
log) rather than blocking.
🪄 Autofix (Beta)
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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 03ec2202-11be-466b-aa71-643e5059c20f
📒 Files selected for processing (8)
browser-features/chrome/common/guided-tour/index.tsbrowser-features/chrome/common/guided-tour/overlay.tsbrowser-features/chrome/common/guided-tour/tour-definitions/workspaces.tsbrowser-features/pages-settings/src/app/splitview/page.tsxbrowser-features/pages-settings/src/components/app-sidebar.tsxbrowser-features/pages-settings/src/components/common/learn-button.tsxbrowser-features/pages-settings/src/lib/i18n/locales/en-US.jsonbrowser-features/pages-settings/src/lib/i18n/locales/ja-JP.json
🚧 Files skipped from review as they are similar to previous changes (5)
- browser-features/pages-settings/src/components/app-sidebar.tsx
- browser-features/chrome/common/guided-tour/index.ts
- browser-features/pages-settings/src/app/splitview/page.tsx
- browser-features/pages-settings/src/lib/i18n/locales/en-US.json
- browser-features/pages-settings/src/lib/i18n/locales/ja-JP.json
There was a problem hiding this comment.
♻️ Duplicate comments (1)
browser-features/pages-settings/src/components/common/learn-button.tsx (1)
14-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing error handling (previously flagged, still unaddressed).
The async RPC call still lacks try/catch wrapping. If
setStringPrefrejects, the promise rejection will be unhandled and provide no diagnostics. As per coding guidelines, "Use try/catch error handling withconsole.error('[FeatureName]', ...)prefix for log messages."🛡️ Recommended fix
const handleStartTour = useCallback(async () => { + try { const payload = JSON.stringify({ tourId }); await rpc.setStringPref("floorp.guidedTour.request", payload); + } catch (error) { + console.error("[GuidedTour]", "Failed to start tour:", error, { tourId }); + } }, [tourId]);🤖 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 `@browser-features/pages-settings/src/components/common/learn-button.tsx` around lines 14 - 17, The async call in handleStartTour lacks error handling; wrap the await rpc.setStringPref("floorp.guidedTour.request", payload) in a try/catch inside handleStartTour and on error log the exception using console.error with the required prefix (e.g. console.error('[LearnButton]', error, { tourId, payload })); ensure the function still returns/continues appropriately after logging.Source: Coding guidelines
🧹 Nitpick comments (2)
browser-features/chrome/common/guided-tour/components/TourOverlay.tsx (1)
150-166: 💤 Low valueWindow resize may cause stale tooltip positioning.
The tooltip position is computed using
window.innerWidthandwindow.innerHeightdirectly in the memo. These values are not reactive, so if the user resizes the browser window during the tour, the tooltip and blocker dimensions won't update until the next step change or target movement.Consider adding a resize signal or using a resize observer for more responsive positioning during window resizes.
🤖 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 `@browser-features/chrome/common/guided-tour/components/TourOverlay.tsx` around lines 150 - 166, tooltipPos currently reads window.innerWidth/innerHeight directly inside the createMemo (via computeTooltipPosition), so resizing the browser leaves tooltip/blocker placement stale; fix by introducing a reactive window-size signal (e.g. createSignal/getWindowSize) or a ResizeObserver and update it on resize, then use that signal inside tooltipPos and any other calculations that depend on viewport size (computeTooltipPosition, tooltipSize, spotlight positioning); ensure you attach/cleanup the resize listener or observer (onCleanup) so the memo recomputes on resize and the tooltip/blocker update immediately.browser-features/chrome/common/guided-tour/index.ts (1)
14-30: ⚡ Quick winController lifecycle is not explicitly managed for cleanup.
The
TourControlleris instantiated ininit()but there's no explicit cleanup when the component is disposed. While the controller usesonCleanup()internally, this only works if the controller is created within a Solid reactive scope (e.g., inside a component render function). Sinceinit()is called outside the render cycle, theonCleanupcallback in the controller constructor may not be triggered on HMR or component disposal.Consider either:
- Moving controller creation into the rendered component, or
- Storing a reference and calling
controller.stop()in a dispose method♻️ Suggested approach
`@noraComponent`(import.meta.hot) export default class GuidedTour extends NoraComponentBase { + private controller: TourController | null = null; + init(): void { if (!document.getElementById("floorp-guided-tour-style")) { const styleEl = document.createElement("style"); styleEl.id = "floorp-guided-tour-style"; styleEl.textContent = style; document.head?.appendChild(styleEl); } - const controller = new TourController(); + this.controller = new TourController(); const mainWindow = document.getElementById("main-window"); if (mainWindow) { - render(createTourOverlay(controller), mainWindow, { + render(createTourOverlay(this.controller), mainWindow, { hotCtx: import.meta.hot, }); } } + + dispose(): void { + this.controller?.stop(); + this.controller = null; + } }🤖 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 `@browser-features/chrome/common/guided-tour/index.ts` around lines 14 - 30, The TourController instance created in init() isn't tied to the Solid render lifecycle so its internal onCleanup won't run; modify init() to ensure explicit cleanup by either moving new TourController() into the component returned by createTourOverlay (so it's created within Solid's reactive scope) or keep the controller reference and call controller.stop() when the overlay is disposed/unmounted (hook into the render dispose or HMR hot.dispose callback used with render(..., { hotCtx: import.meta.hot })). Update references to TourController, init, createTourOverlay, render, and controller.stop accordingly.
🤖 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.
Duplicate comments:
In `@browser-features/pages-settings/src/components/common/learn-button.tsx`:
- Around line 14-17: The async call in handleStartTour lacks error handling;
wrap the await rpc.setStringPref("floorp.guidedTour.request", payload) in a
try/catch inside handleStartTour and on error log the exception using
console.error with the required prefix (e.g. console.error('[LearnButton]',
error, { tourId, payload })); ensure the function still returns/continues
appropriately after logging.
---
Nitpick comments:
In `@browser-features/chrome/common/guided-tour/components/TourOverlay.tsx`:
- Around line 150-166: tooltipPos currently reads window.innerWidth/innerHeight
directly inside the createMemo (via computeTooltipPosition), so resizing the
browser leaves tooltip/blocker placement stale; fix by introducing a reactive
window-size signal (e.g. createSignal/getWindowSize) or a ResizeObserver and
update it on resize, then use that signal inside tooltipPos and any other
calculations that depend on viewport size (computeTooltipPosition, tooltipSize,
spotlight positioning); ensure you attach/cleanup the resize listener or
observer (onCleanup) so the memo recomputes on resize and the tooltip/blocker
update immediately.
In `@browser-features/chrome/common/guided-tour/index.ts`:
- Around line 14-30: The TourController instance created in init() isn't tied to
the Solid render lifecycle so its internal onCleanup won't run; modify init() to
ensure explicit cleanup by either moving new TourController() into the component
returned by createTourOverlay (so it's created within Solid's reactive scope) or
keep the controller reference and call controller.stop() when the overlay is
disposed/unmounted (hook into the render dispose or HMR hot.dispose callback
used with render(..., { hotCtx: import.meta.hot })). Update references to
TourController, init, createTourOverlay, render, and controller.stop
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0d9e98a5-f2f8-40f4-9a20-421ecb081261
📒 Files selected for processing (15)
browser-features/chrome/common/guided-tour/components/TourOverlay.tsxbrowser-features/chrome/common/guided-tour/controller.tsbrowser-features/chrome/common/guided-tour/index.tsbrowser-features/chrome/common/guided-tour/style.cssbrowser-features/chrome/common/guided-tour/test/tourDefinitions.test.tsbrowser-features/chrome/common/guided-tour/tour-definitions/index.tsbrowser-features/chrome/common/guided-tour/tour-definitions/workspaces.tsbrowser-features/chrome/common/guided-tour/types.tsbrowser-features/pages-settings/src/app/splitview/page.tsxbrowser-features/pages-settings/src/components/common/learn-button.tsxbrowser-features/pages-welcome/src/app/features/page.tsxbrowser-features/pages-welcome/src/lib/i18n/locales/en-US.jsonbrowser-features/pages-welcome/src/lib/i18n/locales/ja-JP.jsoni18n/en-US/browser-chrome.jsoni18n/ja-JP/browser-chrome.json
💤 Files with no reviewable changes (1)
- browser-features/pages-settings/src/app/splitview/page.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- browser-features/chrome/common/guided-tour/tour-definitions/workspaces.ts
- browser-features/pages-welcome/src/lib/i18n/locales/en-US.json
- browser-features/pages-welcome/src/app/features/page.tsx
- browser-features/pages-welcome/src/lib/i18n/locales/ja-JP.json
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
browser-features/chrome/common/guided-tour/components/TourOverlay.tsx (1)
24-24: ⚡ Quick winAdd JSDoc comment to
createTourOverlay.The coding guidelines require JSDoc comments on functions explaining purpose and parameters. Adding documentation would clarify that this factory returns a SolidJS component function for rendering the guided-tour overlay.
📝 Proposed JSDoc
+/** + * Create a SolidJS component that renders the guided-tour overlay with spotlight, blockers, and tooltip. + * `@param` controller - TourController instance managing tour state and navigation + * `@returns` SolidJS component function + */ export function createTourOverlay(controller: TourController) {🤖 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 `@browser-features/chrome/common/guided-tour/components/TourOverlay.tsx` at line 24, Add a JSDoc comment for the createTourOverlay function: document its purpose as a factory that returns a SolidJS component function for rendering the guided-tour overlay, describe the controller parameter (TourController) and its role, and specify the return type (a SolidJS component/function that renders the overlay). Place the JSDoc immediately above the export function createTourOverlay(controller: TourController) declaration and include brief param and returns tags to satisfy the coding guidelines.Source: Coding guidelines
browser-features/chrome/common/guided-tour/tooltip-position.ts (1)
22-29: ⚡ Quick winAdd JSDoc comments to exported functions.
The coding guidelines require JSDoc comments on functions to explain purpose and parameters. The exported functions
rectsIntersect,getTooltipRect,computeTooltipPosition,resolveTooltipPosition, andgetChromeObstacleRectslack JSDoc documentation.While the Japanese comment at Line 109-111 explains the fallback logic, using JSDoc format would improve consistency and enable better IDE support.
📝 Example JSDoc addition
+/** + * Check if two rectangles intersect. + * `@param` a - First rectangle + * `@param` b - Second rectangle + * `@returns` true if rectangles overlap + */ export function rectsIntersect(a: TargetRect, b: TargetRect): boolean { +/** + * Convert tooltip position and dimensions to a TargetRect. + * `@param` pos - Tooltip position (left/top) + * `@param` width - Tooltip width + * `@param` height - Tooltip height + * `@returns` Rectangle representing tooltip bounds + */ export function getTooltipRect( +/** + * Compute tooltip position for a given placement, with viewport clamping and flip fallback. + * `@param` rect - Target rectangle to position around (null for center) + * `@param` placement - Preferred placement direction + * `@param` tooltipW - Tooltip width + * `@param` tooltipH - Tooltip height + * `@param` viewportW - Viewport width + * `@param` viewportH - Viewport height + * `@returns` Computed position + */ export function computeTooltipPosition( +/** + * Resolve tooltip position that avoids intersecting obstacle rectangles. + * If all placements intersect obstacles, shifts tooltip below the tallest obstacle. + * `@param` rect - Target rectangle (null for center) + * `@param` preferredPlacement - Preferred placement direction + * `@param` tooltipW - Tooltip width + * `@param` tooltipH - Tooltip height + * `@param` viewportW - Viewport width + * `@param` viewportH - Viewport height + * `@param` obstacles - Array of obstacle rectangles to avoid + * `@returns` Resolved position + */ export function resolveTooltipPosition( +/** + * Collect Chrome UI element rectangles (urlbar, navigator-toolbox) as obstacles. + * `@returns` Array of obstacle rectangles from visible Chrome UI elements + */ export function getChromeObstacleRects(): TargetRect[] {Also applies to: 31-37, 39-99, 112-174, 177-193
🤖 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 `@browser-features/chrome/common/guided-tour/tooltip-position.ts` around lines 22 - 29, Add JSDoc comments to each exported function to describe purpose, parameters, and return values: document rectsIntersect(a: TargetRect, b: TargetRect) explaining it checks axis-aligned rectangle intersection and describing a and b as TargetRect objects and returning boolean; document getTooltipRect(target: ElementOrRect, tooltipSize: Size) describing target input (DOM element or rect) and returned TargetRect; document computeTooltipPosition(anchorRect: TargetRect, tooltipRect: TargetRect, preferredPosition?: Position) describing inputs and returned Position/TargetRect; document resolveTooltipPosition(…) describing fallback logic and return type; and document getChromeObstacleRects(windowOrDocument) describing that it returns obstacle TargetRect[] for positioning; include `@param` and `@returns` tags and a short one-line description for each to follow project JSDoc guidelines.Source: Coding guidelines
browser-features/chrome/common/guided-tour/workspace-panel-guard.ts (1)
24-76: ⚡ Quick winConsider adding JSDoc comments to methods.
The class has a JSDoc comment, but individual methods (
isVisible(),cleanupStale(),ensureOpen()) lack JSDoc documentation explaining their purpose and parameters. According to coding guidelines, functions should have JSDoc comments explaining their purpose and parameters.📝 Example JSDoc additions
/** * ワークスペースパネルが表示されているか確認 * `@returns` パネルが表示されている場合true */ isVisible(): boolean { // ... } /** * 旧実装で残ったロック属性・noautohide を解除する * パネルが改変されていた場合、クローズリスナーを再登録する */ static cleanupStale(): void { // ... } /** * ワークスペースパネルを開く * 既に開いている場合は何もしない * PanelUI.showSubView を優先し、失敗時はボタンクリックにフォールバック */ async ensureOpen(): Promise<void> { // ... }🤖 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 `@browser-features/chrome/common/guided-tour/workspace-panel-guard.ts` around lines 24 - 76, Add JSDoc comments for the three methods on WorkspacePanelGuard: isVisible(), static cleanupStale(), and async ensureOpen() that briefly state each method's purpose and document return type and behavior (use `@returns` for isVisible and ensureOpen where appropriate and note no parameters), mirror the style/language of the existing class comment, and for cleanupStale mention it is static and that it re-registers close listeners when it removes stale attributes; keep comments short and colocated directly above each method declaration.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.
Inline comments:
In `@browser-features/chrome/common/guided-tour/workspace-panel-guard.ts`:
- Around line 12-18: PanelUIType is declared inline and its showSubView
signature differs from the upstream contract; move the type declaration for
PanelUIType into the shared types file (types.ts) and export it, then change
showSubView to match the upstream signature showSubView(id: string, anchor?:
Element | null): Promise<void> (remove the required anchor: Element and the
extra event?: Event parameter). Update any local imports/usages to import the
moved PanelUIType and adjust callers to pass an optional anchor (and stop
passing an event) so the interface and all usages conform to the upstream
contract.
---
Nitpick comments:
In `@browser-features/chrome/common/guided-tour/components/TourOverlay.tsx`:
- Line 24: Add a JSDoc comment for the createTourOverlay function: document its
purpose as a factory that returns a SolidJS component function for rendering the
guided-tour overlay, describe the controller parameter (TourController) and its
role, and specify the return type (a SolidJS component/function that renders the
overlay). Place the JSDoc immediately above the export function
createTourOverlay(controller: TourController) declaration and include brief
param and returns tags to satisfy the coding guidelines.
In `@browser-features/chrome/common/guided-tour/tooltip-position.ts`:
- Around line 22-29: Add JSDoc comments to each exported function to describe
purpose, parameters, and return values: document rectsIntersect(a: TargetRect,
b: TargetRect) explaining it checks axis-aligned rectangle intersection and
describing a and b as TargetRect objects and returning boolean; document
getTooltipRect(target: ElementOrRect, tooltipSize: Size) describing target input
(DOM element or rect) and returned TargetRect; document
computeTooltipPosition(anchorRect: TargetRect, tooltipRect: TargetRect,
preferredPosition?: Position) describing inputs and returned
Position/TargetRect; document resolveTooltipPosition(…) describing fallback
logic and return type; and document getChromeObstacleRects(windowOrDocument)
describing that it returns obstacle TargetRect[] for positioning; include `@param`
and `@returns` tags and a short one-line description for each to follow project
JSDoc guidelines.
In `@browser-features/chrome/common/guided-tour/workspace-panel-guard.ts`:
- Around line 24-76: Add JSDoc comments for the three methods on
WorkspacePanelGuard: isVisible(), static cleanupStale(), and async ensureOpen()
that briefly state each method's purpose and document return type and behavior
(use `@returns` for isVisible and ensureOpen where appropriate and note no
parameters), mirror the style/language of the existing class comment, and for
cleanupStale mention it is static and that it re-registers close listeners when
it removes stale attributes; keep comments short and colocated directly above
each method declaration.
🪄 Autofix (Beta)
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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6291d694-dcaf-4ef5-a7a2-93d8e03fea8a
📒 Files selected for processing (9)
browser-features/chrome/common/guided-tour/components/TourOverlay.tsxbrowser-features/chrome/common/guided-tour/controller.tsbrowser-features/chrome/common/guided-tour/index.tsbrowser-features/chrome/common/guided-tour/test/tooltip-position.test.tsbrowser-features/chrome/common/guided-tour/test/tourDefinitions.test.tsbrowser-features/chrome/common/guided-tour/tooltip-position.tsbrowser-features/chrome/common/guided-tour/tour-definitions/workspaces.tsbrowser-features/chrome/common/guided-tour/types.tsbrowser-features/chrome/common/guided-tour/workspace-panel-guard.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- browser-features/chrome/common/guided-tour/tour-definitions/workspaces.ts
- browser-features/chrome/common/guided-tour/types.ts
- browser-features/chrome/common/guided-tour/test/tourDefinitions.test.ts
- browser-features/chrome/common/guided-tour/controller.ts
Ryosuke-Asano
added a commit
that referenced
this pull request
Jun 13, 2026
Add error handling, pane ratio normalization, PanelUI type alignment, noopener on external links, and minor cleanup from review comments.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
browser-features/pages-settings/src/app/splitview/dataManager.ts (1)
84-109: ⚡ Quick winRedundant pref reads in mount effect — restructure for efficiency.
getSplitViewSettings()at line 88 already readsPREF_SPLIT_VIEW_ENABLEDandPREF_BROWSER_SPLIT_VIEW_ENABLED(lines 63-64), yet lines 92-93 read them again for reconciliation. This doubles I/O and risks race conditions if prefs change between reads.♻️ Proposed refactor
Option 1: Have
getSplitViewSettingsreturn raw pref values alongside the form data:-export async function getSplitViewSettings(): Promise<SplitViewFormData> { +export async function getSplitViewSettings(): Promise<{ + formData: SplitViewFormData; + rawPrefs: { floorpEnabled: boolean | null; browserEnabled: boolean | null }; +}> { const [floorpEnabled, browserEnabled, configStr, paneSizesStr] = await Promise.all([ ... ]); const enabled = resolveEnabledFromPrefs(floorpEnabled, browserEnabled); - return { + return { + formData: { enabled, ...parseSplitViewConfig(configStr), ...parseSplitViewPaneSizes(paneSizesStr), + }, + rawPrefs: { floorpEnabled, browserEnabled }, }; }Then in
useEffect:- const values = await getSplitViewSettings(); - setSettings(values); - const [floorpEnabled, browserEnabled] = await Promise.all([ - rpc.getBoolPref(PREF_SPLIT_VIEW_ENABLED), - rpc.getBoolPref(PREF_BROWSER_SPLIT_VIEW_ENABLED), - ]); + const { formData, rawPrefs } = await getSplitViewSettings(); + setSettings(formData); + const { floorpEnabled, browserEnabled } = rawPrefs; if ( - floorpEnabled !== values.enabled || - browserEnabled !== values.enabled + floorpEnabled !== formData.enabled || + browserEnabled !== formData.enabled ) { - await saveSplitViewSettings(values); + await saveSplitViewSettings(formData); }🤖 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 `@browser-features/pages-settings/src/app/splitview/dataManager.ts` around lines 84 - 109, The mount effect is redundantly re-reading PREF_SPLIT_VIEW_ENABLED and PREF_BROWSER_SPLIT_VIEW_ENABLED after getSplitViewSettings already reads them, so update the code to use the pref values returned by getSplitViewSettings (or change getSplitViewSettings to return { values, floorpEnabled, browserEnabled }) and remove the duplicate rpc.getBoolPref calls in loadSettings; then perform the reconciliation using those returned pref booleans and call saveSplitViewSettings(values) only when they differ. Reference: getSplitViewSettings, saveSplitViewSettings, the loadSettings inner function inside the useEffect, and the PREF_SPLIT_VIEW_ENABLED / PREF_BROWSER_SPLIT_VIEW_ENABLED symbols.browser-features/chrome/common/split-view/service.ts (1)
39-58: ⚡ Quick winSimplify the
ownerparameter type.The type
NonNullable<ReturnType<typeof getOwner>> | nullis redundant becausegetOwner()already returnsOwner | null. TheNonNullablewrapper combined with| nulljust results inOwner | nullagain.♻️ Simplified type signature
private startManager( - owner: NonNullable<ReturnType<typeof getOwner>> | null, + owner: ReturnType<typeof getOwner>, ): void {🤖 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 `@browser-features/chrome/common/split-view/service.ts` around lines 39 - 58, The parameter type for startManager is unnecessarily verbose; replace the current signature owner: NonNullable<ReturnType<typeof getOwner>> | null with a simplified type that matches getOwner's return (e.g., owner: ReturnType<typeof getOwner> or owner: Owner | null) in the startManager method, keeping the same runtime behavior in the body (calls to runWithOwner(owner, ...) remain unchanged).
🤖 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
`@browser-features/pages-settings/src/app/splitview/components/LayoutSettings.tsx`:
- Around line 347-349: The onClick currently invokes the async callback
onLayoutChange(layout.value) without catching errors; update the onClick handler
in LayoutSettings.tsx to await the call inside a try/catch (either make the
handler async or use an async IIFE) and on error call
console.error("[SplitView]", "onLayoutChange failed", error) so any rejection is
handled and logged; refer to the onLayoutChange symbol and the onClick handler
where the invocation occurs.
In
`@browser-features/pages-settings/src/app/splitview/components/PaneSizeSettings.tsx`:
- Around line 76-100: The three updater functions (updateFlexAt, updateGridCol,
updateGridRow) call the async callback onPaneSizesChange without error handling;
wrap each invocation in an async try/catch, await the onPaneSizesChange call,
and on error log with console.error("[PaneSizeSettings]", ...) including the
error and relevant inputs (e.g., index/percent and the computed payload using
fromPercent/normalizeFlexRatios and settings.*) so failures are caught and
diagnostic info is emitted.
- Around line 30-53: The remainder-floor (Math.max(0.05,...)) in
normalizeFlexRatios can produce a derived pane smaller than the UI min; replace
that approach by enforcing a MIN_PANE_RATIO (e.g., 0.1) and, when the computed
remainder < MIN_PANE_RATIO, scale the user-controlled panes down proportionally
so remainder becomes MIN_PANE_RATIO. Concretely: in normalizeFlexRatios (for
maxPanes === 3 and default 4-pane branch) compute clamped controlled values
(using clampRatio), let sumControlled = sum(controlled), let remainder = 1 -
sumControlled; if remainder < MIN_PANE_RATIO then compute scale = (1 -
MIN_PANE_RATIO) / sumControlled and multiply each controlled by scale and set
remainder = MIN_PANE_RATIO; finally compute the final ratios (optionally
normalize to sum 1) and return them; keep clampRatio usage for initial clamping.
---
Nitpick comments:
In `@browser-features/chrome/common/split-view/service.ts`:
- Around line 39-58: The parameter type for startManager is unnecessarily
verbose; replace the current signature owner: NonNullable<ReturnType<typeof
getOwner>> | null with a simplified type that matches getOwner's return (e.g.,
owner: ReturnType<typeof getOwner> or owner: Owner | null) in the startManager
method, keeping the same runtime behavior in the body (calls to
runWithOwner(owner, ...) remain unchanged).
In `@browser-features/pages-settings/src/app/splitview/dataManager.ts`:
- Around line 84-109: The mount effect is redundantly re-reading
PREF_SPLIT_VIEW_ENABLED and PREF_BROWSER_SPLIT_VIEW_ENABLED after
getSplitViewSettings already reads them, so update the code to use the pref
values returned by getSplitViewSettings (or change getSplitViewSettings to
return { values, floorpEnabled, browserEnabled }) and remove the duplicate
rpc.getBoolPref calls in loadSettings; then perform the reconciliation using
those returned pref booleans and call saveSplitViewSettings(values) only when
they differ. Reference: getSplitViewSettings, saveSplitViewSettings, the
loadSettings inner function inside the useEffect, and the
PREF_SPLIT_VIEW_ENABLED / PREF_BROWSER_SPLIT_VIEW_ENABLED symbols.
🪄 Autofix (Beta)
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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e75d6c9a-65dc-4b82-974f-164131c8afbf
📒 Files selected for processing (42)
browser-features/chrome/common/guided-tour/components/TourOverlay.tsxbrowser-features/chrome/common/guided-tour/controller.tsbrowser-features/chrome/common/guided-tour/workspace-panel-guard.tsbrowser-features/chrome/common/mouse-gesture/utils/ui-toggle.tsbrowser-features/chrome/common/split-view/components/split-view-pane-drag.tsbrowser-features/chrome/common/split-view/components/split-view-toolbar-button.tsxbrowser-features/chrome/common/split-view/data/config.tsbrowser-features/chrome/common/split-view/data/enabled.tsbrowser-features/chrome/common/split-view/data/types.tsbrowser-features/chrome/common/split-view/index.tsbrowser-features/chrome/common/split-view/patches/activate-split-pane-browsers.tsbrowser-features/chrome/common/split-view/patches/active-pane-tracker.tsbrowser-features/chrome/common/split-view/patches/context-menu.tsbrowser-features/chrome/common/split-view/patches/event-listeners.tsbrowser-features/chrome/common/split-view/patches/session-restore.tsbrowser-features/chrome/common/split-view/patches/split-view-diagnostics.tsbrowser-features/chrome/common/split-view/patches/tabpanels-patch.tsbrowser-features/chrome/common/split-view/patches/wrapper-patch.tsbrowser-features/chrome/common/split-view/service.tsbrowser-features/chrome/common/split-view/split-view-manager.tsbrowser-features/chrome/common/split-view/styles/split-view.cssbrowser-features/chrome/common/split-view/test/enabled.test.tsbrowser-features/chrome/common/split-view/test/splitViewUtilities.test.tsbrowser-features/chrome/common/split-view/utils/group-layout-store.spec.tsbrowser-features/chrome/common/split-view/utils/group-layout-store.tsbrowser-features/chrome/common/split-view/utils/reorder-panes.tsbrowser-features/chrome/common/split-view/utils/reorder-strip-impl.tsbrowser-features/pages-newtab/src/components/TipBanner/index.tsxbrowser-features/pages-settings/src/app/splitview/components/BasicSettings.tsxbrowser-features/pages-settings/src/app/splitview/components/LayoutSettings.tsxbrowser-features/pages-settings/src/app/splitview/components/PaneSizeSettings.tsxbrowser-features/pages-settings/src/app/splitview/dataManager.test.tsbrowser-features/pages-settings/src/app/splitview/dataManager.tsbrowser-features/pages-settings/src/app/splitview/page.tsxbrowser-features/pages-settings/src/app/splitview/splitViewSettingsLogic.tsbrowser-features/pages-settings/src/components/common/learn-button.tsxbrowser-features/pages-settings/src/lib/i18n/locales/en-US.jsonbrowser-features/pages-settings/src/lib/i18n/locales/ja-JP.jsonbrowser-features/pages-welcome/src/app/features/page.tsxi18n/en-US/browser-chrome.jsoni18n/ja-JP/browser-chrome.jsonstatic/gecko/pref/override.ini
✅ Files skipped from review due to trivial changes (13)
- browser-features/chrome/common/split-view/components/split-view-toolbar-button.tsx
- browser-features/chrome/common/split-view/data/config.ts
- browser-features/chrome/common/split-view/patches/event-listeners.ts
- browser-features/chrome/common/split-view/patches/activate-split-pane-browsers.ts
- browser-features/chrome/common/split-view/data/types.ts
- browser-features/chrome/common/split-view/test/splitViewUtilities.test.ts
- browser-features/chrome/common/split-view/patches/wrapper-patch.ts
- browser-features/chrome/common/split-view/patches/active-pane-tracker.ts
- browser-features/chrome/common/split-view/utils/reorder-strip-impl.ts
- browser-features/chrome/common/split-view/patches/tabpanels-patch.ts
- browser-features/chrome/common/split-view/components/split-view-pane-drag.ts
- i18n/ja-JP/browser-chrome.json
- browser-features/chrome/common/split-view/utils/group-layout-store.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- i18n/en-US/browser-chrome.json
- browser-features/pages-settings/src/components/common/learn-button.tsx
- browser-features/pages-welcome/src/app/features/page.tsx
- browser-features/pages-settings/src/lib/i18n/locales/ja-JP.json
- browser-features/pages-settings/src/lib/i18n/locales/en-US.json
- browser-features/chrome/common/guided-tour/components/TourOverlay.tsx
- browser-features/pages-newtab/src/components/TipBanner/index.tsx
- browser-features/chrome/common/guided-tour/controller.ts
Comment on lines
+347
to
+349
| onClick={() => { | ||
| void onLayoutChange(layout.value); | ||
| }} |
There was a problem hiding this comment.
Add error handling around async callback invocation.
The onClick handler fires the async onLayoutChange callback without catching errors. Unhandled promise rejections can occur if the callback fails.
🛡️ Proposed fix
- onClick={() => {
- void onLayoutChange(layout.value);
- }}
+ onClick={async () => {
+ try {
+ await onLayoutChange(layout.value);
+ } catch (error) {
+ console.error("[SplitView]", "Failed to update layout", error);
+ }
+ }}As per coding guidelines: "Use error handling with try/catch and log messages with console.error("[FeatureName]", ...) prefix".
📝 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.
Suggested change
| onClick={() => { | |
| void onLayoutChange(layout.value); | |
| }} | |
| onClick={async () => { | |
| try { | |
| await onLayoutChange(layout.value); | |
| } catch (error) { | |
| console.error("[SplitView]", "Failed to update layout", error); | |
| } | |
| }} |
🤖 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
`@browser-features/pages-settings/src/app/splitview/components/LayoutSettings.tsx`
around lines 347 - 349, The onClick currently invokes the async callback
onLayoutChange(layout.value) without catching errors; update the onClick handler
in LayoutSettings.tsx to await the call inside a try/catch (either make the
handler async or use an async IIFE) and on error call
console.error("[SplitView]", "onLayoutChange failed", error) so any rejection is
handled and logged; refer to the onLayoutChange symbol and the onClick handler
where the invocation occurs.
Source: Coding guidelines
Comment on lines
+30
to
+53
| function normalizeFlexRatios( | ||
| maxPanes: number, | ||
| rawValues: number[], | ||
| ): number[] { | ||
| if (maxPanes === 2) { | ||
| const first = clampRatio(rawValues[0] ?? 0.5); | ||
| return [first, 1 - first]; | ||
| } | ||
|
|
||
| if (maxPanes === 3) { | ||
| const first = clampRatio(rawValues[0] ?? 0.33); | ||
| const second = clampRatio(rawValues[1] ?? 0.33); | ||
| const third = Math.max(0.05, 1 - first - second); | ||
| const sum = first + second + third; | ||
| return [first / sum, second / sum, third / sum]; | ||
| } | ||
|
|
||
| const first = clampRatio(rawValues[0] ?? 0.25); | ||
| const second = clampRatio(rawValues[1] ?? 0.25); | ||
| const third = clampRatio(rawValues[2] ?? 0.25); | ||
| const fourth = Math.max(0.05, 1 - first - second - third); | ||
| const sum = first + second + third + fourth; | ||
| return [first / sum, second / sum, third / sum, fourth / sum]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any design docs or comments explaining the 0.05 minimum choice.
rg -nC3 'Math\.max\(0\.05' browser-features/pages-settings/src/app/splitview/Repository: Floorp-Projects/Floorp
Length of output: 1865
Fix derived pane min-size mismatch in normalizeFlexRatios (3/4 pane modes)
PaneSizeSettings.tsx computes the derived remainder pane using Math.max(0.05, ...), so when the two/three user-controlled panes are near the upper clamp, the remainder can end up far below the UI’s intended minimum (e.g., ~3% with 3 panes and 0.8/0.8 inputs). Remove/raise the 0.05 floor or rescale controlled panes to guarantee the remainder meets the advertised min (e.g., 10%) after normalization. No nearby docs/comments justify the 0.05 choice.
🤖 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
`@browser-features/pages-settings/src/app/splitview/components/PaneSizeSettings.tsx`
around lines 30 - 53, The remainder-floor (Math.max(0.05,...)) in
normalizeFlexRatios can produce a derived pane smaller than the UI min; replace
that approach by enforcing a MIN_PANE_RATIO (e.g., 0.1) and, when the computed
remainder < MIN_PANE_RATIO, scale the user-controlled panes down proportionally
so remainder becomes MIN_PANE_RATIO. Concretely: in normalizeFlexRatios (for
maxPanes === 3 and default 4-pane branch) compute clamped controlled values
(using clampRatio), let sumControlled = sum(controlled), let remainder = 1 -
sumControlled; if remainder < MIN_PANE_RATIO then compute scale = (1 -
MIN_PANE_RATIO) / sumControlled and multiply each controlled by scale and set
remainder = MIN_PANE_RATIO; finally compute the final ratios (optionally
normalize to sum 1) and return them; keep clampRatio usage for initial clamping.
Comment on lines
+76
to
+100
| const updateFlexAt = (index: number, percent: number) => { | ||
| const rawValues = [...settings.flexRatios]; | ||
| rawValues[index] = fromPercent(percent); | ||
| void onPaneSizesChange({ | ||
| flexRatios: normalizeFlexRatios(settings.maxPanes, rawValues), | ||
| gridColRatio: settings.gridColRatio, | ||
| gridRowRatio: settings.gridRowRatio, | ||
| }); | ||
| }; | ||
|
|
||
| const updateGridCol = (percent: number) => { | ||
| void onPaneSizesChange({ | ||
| flexRatios: settings.flexRatios, | ||
| gridColRatio: fromPercent(percent), | ||
| gridRowRatio: settings.gridRowRatio, | ||
| }); | ||
| }; | ||
|
|
||
| const updateGridRow = (percent: number) => { | ||
| void onPaneSizesChange({ | ||
| flexRatios: settings.flexRatios, | ||
| gridColRatio: settings.gridColRatio, | ||
| gridRowRatio: fromPercent(percent), | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Add error handling around async callback invocations.
The three update functions (updateFlexAt, updateGridCol, updateGridRow) fire the async onPaneSizesChange callback without catching errors. Unhandled promise rejections can occur if the callback fails.
🛡️ Proposed fix
const updateFlexAt = (index: number, percent: number) => {
const rawValues = [...settings.flexRatios];
rawValues[index] = fromPercent(percent);
- void onPaneSizesChange({
- flexRatios: normalizeFlexRatios(settings.maxPanes, rawValues),
- gridColRatio: settings.gridColRatio,
- gridRowRatio: settings.gridRowRatio,
- });
+ onPaneSizesChange({
+ flexRatios: normalizeFlexRatios(settings.maxPanes, rawValues),
+ gridColRatio: settings.gridColRatio,
+ gridRowRatio: settings.gridRowRatio,
+ }).catch((error) => {
+ console.error("[SplitView]", "Failed to update flex ratios", error);
+ });
};
const updateGridCol = (percent: number) => {
- void onPaneSizesChange({
- flexRatios: settings.flexRatios,
- gridColRatio: fromPercent(percent),
- gridRowRatio: settings.gridRowRatio,
- });
+ onPaneSizesChange({
+ flexRatios: settings.flexRatios,
+ gridColRatio: fromPercent(percent),
+ gridRowRatio: settings.gridRowRatio,
+ }).catch((error) => {
+ console.error("[SplitView]", "Failed to update grid column ratio", error);
+ });
};
const updateGridRow = (percent: number) => {
- void onPaneSizesChange({
- flexRatios: settings.flexRatios,
- gridColRatio: settings.gridColRatio,
- gridRowRatio: fromPercent(percent),
- });
+ onPaneSizesChange({
+ flexRatios: settings.flexRatios,
+ gridColRatio: settings.gridColRatio,
+ gridRowRatio: fromPercent(percent),
+ }).catch((error) => {
+ console.error("[SplitView]", "Failed to update grid row ratio", error);
+ });
};As per coding guidelines: "Use error handling with try/catch and log messages with console.error("[FeatureName]", ...) prefix".
🤖 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
`@browser-features/pages-settings/src/app/splitview/components/PaneSizeSettings.tsx`
around lines 76 - 100, The three updater functions (updateFlexAt, updateGridCol,
updateGridRow) call the async callback onPaneSizesChange without error handling;
wrap each invocation in an async try/catch, await the onPaneSizesChange call,
and on error log with console.error("[PaneSizeSettings]", ...) including the
error and relevant inputs (e.g., index/percent and the computed payload using
fromPercent/normalizeFlexRatios and settings.*) so failures are caught and
diagnostic info is emitted.
Source: Coding guidelines
- Implemented Split View settings page with layout and pane size options. - Created Tip Banner component to provide users with feature tips. - Added BasicSettings and LayoutSettings components for managing Split View preferences. - Introduced dataManager for handling Split View settings persistence. - Enhanced welcome page with Split View feature introduction. - Developed HelpSection and LearnButton components for contextual help and tutorials. - Added TutorialModal for interactive user guidance on new features. - Included SVG asset for visual representation of Split View. - Updated translations for new features and tips.
- Added GestureCanvas component for interactive gesture drawing. - Created SplitViewSandbox component for layout manipulation. - Developed WorkspaceGuide component for workspace navigation. - Expanded tutorial steps for mouse gestures and split view features. - Updated localization files for new tutorial content.
- Fix workspace tutorial text to match actual UI (button position, context menu, archive/restore flow) - Add workspace tutorial steps for containers, archive/restore, and sidebar mode with animated visuals - Rewrite WorkspaceGuide mockup to show popup panel instead of sidebar - Fix gesture tutorial: add 'right-click' to step 2-4 instructions - Fix modal layout order: title → description → content - Reduce GestureCanvas height from 200px to 140px - Unify 'Split View' → '分割ビュー' across all ja-JP locales
…ures, and split view - Implemented a guided tour system to help users understand new features. - Added localized strings for English and Japanese languages. - Created tour definitions for workspaces, mouse gestures, and split view. - Developed overlay and navigation components for the guided tour. - Integrated actions such as click, right-click, and scroll into view for interactive steps. - Included functionality to manage tour state and preferences.
…ssues - Fix LearnButton payload key from 'step' to 'currentStep' - Add pref normalization to accept both currentStep and legacy step fields - Add typeof guard for step index validation - Add tour-active guard for waitForSelector timeout - Increase z-index to 999999 for better layering - Improve spotlight with border and background for better visibility - Fix resize repositioning to recalculate both spotlight and tooltip - Remove debug logs
- Use moveTo() instead of hidePopup/openPopup for spotlight repositioning to prevent XUL popupManager cascade-closing the workspace panel - Reorder action execution before waitForSelector to avoid timing issues - Add polling-based visibility check with XUL state detection - Guard against duplicate showStep calls and remove redundant calls from onNext/onPrev callbacks (delegate to pref observer) - Add panel health check timer with auto-recovery for disappeared tooltips - Fix workspaces tour: step 2 targets panel with waitForSelector, step 4 removes rightClick action (user performs manually) - i18n: use locale keys for tour button labels (skip/prev/next/finish) - i18n: add badges.new key for sidebar New badge - Fix splitview page: use window focus event instead of documentElement onfocus
…nents - Introduced a new TourController class to manage the guided tour state and navigation. - Created TourOverlay component for rendering the tour interface with tooltips and spotlight effects. - Added CSS styles for the guided tour overlay and tooltip elements. - Updated tour definitions to include new properties for tooltip placement and actions. - Removed obsolete overlay implementation and integrated new functionality into the TourController. - Added unit tests for tour definitions to ensure proper functionality and structure.
…our steps - Introduced `keepWorkspacePanelOpen` property in `TourStep` interface to prevent the workspace panel from closing during specific tour steps. - Updated `TourController` to manage workspace panel state, ensuring it remains open when required. - Implemented `syncWorkspacePanelForStep` and `scheduleWorkspacePanelOpen` methods for better synchronization of the workspace panel with tour steps. - Modified tour definitions for workspaces to utilize the new `keepWorkspacePanelOpen` feature. - Added unit tests to verify the behavior of workspace panel management during guided tour steps.
…e settings management - Removed redundant split view initialization logic from the SplitView component, delegating to the splitViewService for better encapsulation. - Updated SplitViewManager to improve method visibility and organization. - Refactored context menu integration to utilize the new isSplitViewEnabled function for preference checks. - Enhanced dataManager for split view settings to improve validation and state management. - Cleaned up CSS styles for better readability and maintainability. - Improved test cases for split view utilities to ensure robustness.
Add error handling, pane ratio normalization, PanelUI type alignment, noopener on external links, and minor cleanup from review comments.
Fix deno lint issues (window prefix, unused imports, require-await), update enabled.test.ts for runTests API, and use style.getPropertyValue in ui-toggle for Deno CSSStyleDeclaration compatibility.
…mers CI type check rejects pushing Timeout handles into number[] on Linux.
Ryosuke-Asano
force-pushed
the
feature/guided-tour-overlay
branch
from
June 13, 2026 23:08
c42a7c5 to
42f8589
Compare
This commit deletes the test file for the split view data manager, which included various test cases for preference resolution, configuration parsing, and validation of form data. The removal is part of a cleanup effort to streamline the testing suite.
assertEquals uses referential comparison, so object assertions failed in CI.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Enhancements
Localization
Tests