Skip to content

ntp: omnibar states #1882

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion special-pages/pages/new-tab/app/entry-points/omnibar.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import { h } from 'preact';
import { Centered } from '../components/Layout.js';
import { OmnibarCustomized } from '../omnibar/components/OmnibarCustomized.js';
import { PersistentTermsProvider } from '../omnibar/components/PersistentTermsProvider.js';

export function factory() {
return (
<Centered data-entry-point="omnibar">
<OmnibarCustomized />
<PersistentTermsProvider>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This state needs to live for the lifespan of the page, not the widget, so we go 1 level higher

<OmnibarCustomized />
</PersistentTermsProvider>
</Centered>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { SearchForm } from './SearchForm';
import { SearchFormProvider } from './SearchFormProvider';
import { SuggestionsList } from './SuggestionsList';
import { TabSwitcher } from './TabSwitcher';
import { useQueryWithPersistence } from './PersistentTermsProvider.js';

/**
* @typedef {import('../strings.json')} Strings
Expand All @@ -27,7 +28,7 @@ import { TabSwitcher } from './TabSwitcher';
export function Omnibar({ mode, setMode, enableAi }) {
const { t } = useTypedTranslationWith(/** @type {Strings} */ ({}));

const [query, setQuery] = useState(/** @type {String} */ (''));
const [query, setQuery] = useQueryWithPersistence();
const [resetKey, setResetKey] = useState(0);
const [autoFocus, setAutoFocus] = useState(false);
const [focusRing, setFocusRing] = useState(/** @type {boolean|undefined} */ (undefined));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import { ArrowIndentCenteredIcon } from '../../components/Icons.js';
export function OmnibarConsumer() {
const { state } = useContext(OmnibarContext);
if (state.status === 'ready') {
return <OmnibarReadyState config={state.config} />;
return <OmnibarReadyState config={state.config} key={state.config.tabId} />;
}
return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { h } from 'preact';

import { OmnibarConsumer } from './OmnibarConsumer.js';
import { SearchIcon } from '../../components/Icons.js';
import { PruneInitialTerms } from './PersistentTermsProvider.js';

/**
* @import enStrings from "../strings.json"
Expand Down Expand Up @@ -37,6 +38,7 @@ export function OmnibarCustomized() {
return (
<OmnibarProvider>
<OmnibarConsumer />
<PruneInitialTerms />
</OmnibarProvider>
);
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { createContext, h } from 'preact';
import { useCallback, useEffect, useReducer, useRef } from 'preact/hooks';
import { useCallback, useContext, useEffect, useReducer, useRef } from 'preact/hooks';
import { useMessaging } from '../../types.js';
import { reducer, useInitialDataAndConfig, useConfigSubscription } from '../../service.hooks.js';
import { OmnibarService } from '../omnibar.service.js';
Expand Down Expand Up @@ -153,7 +153,7 @@ export function OmnibarProvider(props) {
/**
* @return {import("preact").RefObject<OmnibarService>}
*/
export function useService() {
function useService() {
const service = useRef(/** @type {OmnibarService|null} */ (null));
const ntp = useMessaging();
useEffect(() => {
Expand All @@ -165,3 +165,7 @@ export function useService() {
}, [ntp]);
return service;
}

export function useOmnibarService() {
return useContext(OmnibarServiceContext);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all of the naming here is a mess - can be addressed after the POC is confirmed working

Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import { h, createContext } from 'preact';
import { useCallback, useContext, useEffect, useState } from 'preact/hooks';
import { OmnibarTerm } from '../omnibar.term.js';
import { OmnibarContext, useOmnibarService } from './OmnibarProvider.js';

const InitialTermContext = createContext(/** @type {OmnibarTerm|null} */ (null));

/**
* @param {object} props
* @param {import('preact').ComponentChildren} props.children
*/
export function PersistentTermsProvider({ children }) {
const [terms] = useState(() => new OmnibarTerm());
return <InitialTermContext.Provider value={terms}>{children}</InitialTermContext.Provider>;
}

/**
* When the Omnibar Service becomes ready, subscribe to tab updates so that we can prune old state
*/
export function PruneInitialTerms() {
const terms = useContext(InitialTermContext);
const service = useOmnibarService();
useEffect(() => {
return service?.onConfig(({ source, data }) => {
if (source === 'subscription') {
if (typeof data.tabId === 'string' && Array.isArray(data.tabIds)) {
terms?.prune({ preserve: data.tabIds });
}
}
});
}, [service]);
return null;
}

/**
* A normal set-state, but with values recorded. Must be used when the Omnibar Service is ready
*/
export function useQueryWithPersistence() {
const terms = useContext(InitialTermContext);
const { state } = useContext(OmnibarContext);

if (state.status !== 'ready') {
throw new Error('Cannot use `useQueryWithPersistence` without Omnibar Service being ready.');
}

const [query, _setQuery] = useState(() => terms?.byId(state.config.tabId) || '');

/** @type {(term: string) => void} */
const setter = useCallback(
(term) => {
if (state.config.tabId) {
terms?.update({ id: state.config.tabId, term });
}
_setQuery(term);
},
[state.config.tabId],
);

return /** @type {const} */ ([query, setter]);
}
Original file line number Diff line number Diff line change
Expand Up @@ -156,4 +156,29 @@ export class OmnibarPage {
const calls = await this.ntp.mocks.outgoing({ names: [method] });
expect(calls).toHaveLength(0);
}

async didSwitchToTab(id, tabs) {
await this.ntp.mocks.simulateSubscriptionMessage(
sub('omnibar_onConfigUpdate'),
config({
tabId: id,
tabIds: tabs,
mode: 'search',
}),
);
}
}

/**
* @param {import("../../../types/new-tab.js").NewTabMessages["subscriptions"]["subscriptionEvent"]} name
*/
function sub(name) {
return name;
}

/**
* @param {import("../../../types/new-tab.js").OmnibarConfig} c
*/
function config(c) {
return c;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { test } from '@playwright/test';
import { NewtabPage } from '../../../integration-tests/new-tab.page.js';
import { OmnibarPage } from './omnibar.page.js';

test.describe('omnibar widget persistence', () => {
test('remembers input across tabs', async ({ page }, workerInfo) => {
const ntp = NewtabPage.create(page, workerInfo);
const omnibar = new OmnibarPage(ntp);
await ntp.reducedMotion();
await ntp.openPage({ additional: { omnibar: true, 'omnibar.tabs': true } });
await omnibar.ready();

// first fill
await omnibar.searchInput().fill('shoes');

// switch
await omnibar.didSwitchToTab('02', ['01', '02']);
await omnibar.expectInputValue('');

// second fill
await omnibar.searchInput().fill('dresses');

// back to first
await omnibar.didSwitchToTab('01', ['01', '02']);
await omnibar.expectInputValue('shoes');

// back to second again
await omnibar.didSwitchToTab('02', ['01', '02']);
await omnibar.expectInputValue('dresses');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ export function omnibarMockTransport() {
if (showAiSettingOverride === 'true' || showAiSettingOverride === 'false') {
config.showAiSetting = showAiSettingOverride === 'true';
}
if (url.searchParams.has('omnibar.tabs')) {
config.tabId = '01';
config.tabIds = ['01'];
}
return config;
}
case 'omnibar_getSuggestions': {
Expand Down
29 changes: 29 additions & 0 deletions special-pages/pages/new-tab/app/omnibar/omnibar.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,16 @@ title: Omnibar Widget
}
```

Note: On Windows `tabId` and `tabIds` are required:

```json
{
"mode": "search",
"tabId": "01",
"tabIds": ["01"]
}
```

### `omnibar_getSuggestions`
- {@link "NewTab Messages".OmnibarGetSuggestionsRequest}
- Used to fetch search suggestions based on user input
Expand Down Expand Up @@ -75,6 +85,25 @@ title: Omnibar Widget
- {@link "NewTab Messages".OmnibarOnConfigUpdateSubscription}
- The omnibar widget configuration updates
- returns {@link "NewTab Messages".OmnibarConfig}
- Note: tabId and tabIds are required fields on Windows

```json
{
"mode": "ai",
"tabId": "01",
"tabIds": ["01"]
}
```

You can force the frontend to switch to data for a particular tab like this:

```json
{
"mode": "ai",
"tabId": "01",
"tabIds": ["01", "02", "03"]
}
```

## Notifications:

Expand Down
62 changes: 62 additions & 0 deletions special-pages/pages/new-tab/app/omnibar/omnibar.term.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
export class OmnibarTerm {
#values = new Map();

name() {
return 'OmnibarTerm';
}

/**
* Updates the value associated with a given identifier.
*
* @param {object} args
* @param {string} args.id
* @param {string} args.term
*/
update({ id, term }) {
if (string(id) && string(term)) {
this.#values.set(id, term);
}
}

/**
* @param {object} params
* @param {string[]} params.preserve
*/
prune({ preserve }) {
for (const key of this.#values.keys()) {
if (!preserve.includes(key)) {
this.#values.delete(key);
}
}
}

/**
* @param {object} args
* @param {string} args.id
*/
remove({ id }) {
if (string(id)) {
this.#values.delete(id);
}
}

/**
* @param {string|null|undefined} id
* @return {string}
*/
byId(id) {
if (!string(id)) return '';
const value = this.#values.get(id);
if (!string(value)) return '';
return value;
}
}

/**
* @param {unknown} input
*/
function string(input) {
if (typeof input !== 'string') return '';
if (input.trim().length < 1) return '';
return input;
}
19 changes: 19 additions & 0 deletions special-pages/pages/new-tab/messages/types/omnibar-config.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,25 @@
"mode"
],
"properties": {
"tabId": {
"oneOf": [{
"type": "null"
}, {
"title": "Optional Tab Id",
"type": "string"
}]
},
"tabIds": {
"oneOf": [{
"type": "null"
}, {
"title": "Optional List of tab Ids",
"type": "array",
"items": {
"type": "string"
}
}]
},
"mode": {
"title": "Omnibar Mode",
"type": "string",
Expand Down
4 changes: 4 additions & 0 deletions special-pages/pages/new-tab/types/new-tab.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ export type Suggestion =
| WebsiteSuggestion
| HistoryEntrySuggestion
| InternalPageSuggestion;
export type OptionalTabId = string;
export type OptionalListOfTabIds = string[];
export type OmnibarMode = "search" | "ai";
export type EnableDuckAi = boolean;
export type ShowDuckAiSetting = boolean;
Expand Down Expand Up @@ -506,6 +508,8 @@ export interface OmnibarSetConfigNotification {
params: OmnibarConfig;
}
export interface OmnibarConfig {
tabId?: null | OptionalTabId;
tabIds?: null | OptionalListOfTabIds;
mode: OmnibarMode;
enableAi?: EnableDuckAi;
showAiSetting?: ShowDuckAiSetting;
Expand Down
1 change: 1 addition & 0 deletions special-pages/playwright.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export default defineConfig({
'protections.spec.js',
'protections.screenshots.spec.js',
'omnibar.spec.js',
'omnibar.persistence.spec.js',
],
use: {
...devices['Desktop Chrome'],
Expand Down
Loading