Merge pull request #1196 from hypothesis/use-main-frame-main-url-for-group-search

Always use main URL from main frame when searching for groups

src/sidebar/services/groups.js

@@ -35,15 +35,11 @@ function groups(
 
   function getDocumentUriForGroupSearch() {
     function mainUri() {
-      const uris = store.searchUris();
-      if (uris.length === 0) {
+      const mainFrame = store.mainFrame();
+      if (!mainFrame) {
         return null;
       }
-
-      // We get the first HTTP URL here on the assumption that group scopes must
-      // be domains (+paths)? and therefore we need to look up groups based on
-      // HTTP URLs (so eg. we cannot use a "file:" URL or PDF fingerprint).
-      return uris.find(uri => uri.startsWith('http'));
+      return mainFrame.uri;
     }
     return awaitStateChange(store, mainUri);
   }

src/sidebar/services/test/groups-test.js

@@ -57,7 +57,7 @@ describe('groups', function() {
 
     fakeStore = fakeReduxStore(
       {
-        searchUris: ['http://example.org'],
+        mainFrame: { uri: 'http://example.org' },
         focusedGroup: null,
         groups: [],
         directLinkedGroupId: null,
@@ -76,8 +76,8 @@ describe('groups', function() {
         focusedGroup() {
           return this.getState().focusedGroup;
         },
-        searchUris() {
-          return this.getState().searchUris;
+        mainFrame() {
+          return this.getState().mainFrame;
         },
         focusedGroupId() {
           const group = this.getState().focusedGroup;
@@ -388,9 +388,9 @@ describe('groups', function() {
       it('waits for the document URL to be determined', () => {
         const svc = service();
 
-        fakeStore.setState({ searchUris: [] });
+        fakeStore.setState({ mainFrame: null });
         const loaded = svc.load();
-        fakeStore.setState({ searchUris: ['https://asite.com'] });
+        fakeStore.setState({ mainFrame: { uri: 'https://asite.com' } });
 
         return loaded.then(() => {
           assert.calledWith(fakeApi.groups.list, {
@@ -407,7 +407,7 @@ describe('groups', function() {
       });
 
       it('does not wait for the document URL', () => {
-        fakeStore.setState({ searchUris: [] });
+        fakeStore.setState({ mainFrame: null });
         const svc = service();
         return svc.load().then(() => {
           assert.calledWith(fakeApi.groups.list, {
@@ -775,14 +775,16 @@ describe('groups', function() {
       it('should refetch groups if main frame URL has changed', () => {
         const svc = service();
 
-        fakeStore.setState({ searchUris: ['https://domain.com/page-a'] });
+        fakeStore.setState({ mainFrame: { uri: 'https://domain.com/page-a' } });
         return svc
           .load()
           .then(() => {
             // Simulate main frame URL change, eg. due to client-side navigation in
             // a single page application.
             fakeApi.groups.list.resetHistory();
-            fakeStore.setState({ searchUris: ['https://domain.com/page-b'] });
+            fakeStore.setState({
+              mainFrame: { uri: 'https://domain.com/page-b' },
+            });
 
             return fakeRootScope.eventCallbacks[events.FRAME_CONNECTED]();
           })
@@ -794,7 +796,7 @@ describe('groups', function() {
       it('should not refetch groups if main frame URL has not changed', () => {
         const svc = service();
 
-        fakeStore.setState({ searchUris: ['https://domain.com/page-a'] });
+        fakeStore.setState({ mainFrame: { uri: 'https://domain.com/page-a' } });
         return svc
           .load()
           .then(() => {

src/sidebar/store/modules/frames.js

 'use strict';
 
+const { createSelector } = require('reselect');
+
 const util = require('../util');
 
 function init() {
@@ -71,6 +73,24 @@ function frames(state) {
   return state.frames;
 }
 
+/**
+ * Return the "main" frame that the sidebar is connected to.
+ *
+ * The sidebar may be connected to multiple frames from different URLs.
+ * For some purposes, the main frame (typically the top-level one that contains
+ * the sidebar) needs to be distinguished. This selector returns the main frame
+ * for that purpose.
+ *
+ * This may be `null` during startup.
+ */
+const mainFrame = createSelector(
+  state => state.frames,
+
+  // Sub-frames will all have a "frame identifier" set. The main frame is the
+  // one with a `null` id.
+  frames => frames.find(f => !f.id) || null
+);
+
 function searchUrisForFrame(frame) {
   let uris = [frame.uri];
 
@@ -113,6 +133,7 @@ module.exports = {
 
   selectors: {
     frames,
+    mainFrame,
     searchUris,
   },
 };

src/sidebar/store/modules/test/frames-test.js

 'use strict';
 
 const frames = require('../frames');
+const createStore = require('../../create-store');
+
 const session = require('../session');
 const util = require('../../util');
 const unroll = require('../../../../shared/test/util').unroll;
@@ -13,7 +15,16 @@ function init() {
   return Object.assign({}, frames.init(), session.init());
 }
 
-describe('frames reducer', function() {
+describe('sidebar/store/modules/frames', function() {
+  let store;
+
+  beforeEach(() => {
+    // Setup a store for tests. Note that some of the tests in this module
+    // pre-date the `createStore` helper and have not been refactored to use
+    // it yet.
+    store = createStore([frames]);
+  });
+
   describe('#connectFrame', function() {
     it('adds the frame to the list of connected frames', function() {
       const frame = { uri: 'http://example.com' };
@@ -71,6 +82,34 @@ describe('frames reducer', function() {
     });
   });
 
+  describe('#mainFrame', () => {
+    it('returns `null` if no frames are connected', () => {
+      assert.isNull(store.mainFrame());
+    });
+
+    [
+      {
+        frames: [{ id: null, uri: 'https://example.org' }],
+        expectedFrame: 0,
+      },
+      {
+        frames: [
+          // An iframe which is also connected.
+          { id: 'iframe1', uri: 'https://foo.com/' },
+
+          // The top-level frame.
+          { id: null, uri: 'https://example.org' },
+        ],
+        expectedFrame: 1,
+      },
+    ].forEach(({ frames, expectedFrame }) => {
+      it('returns the main frame from the frames connected to the sidebar', () => {
+        frames.forEach(frame => store.connectFrame(frame));
+        assert.equal(store.mainFrame(), frames[expectedFrame]);
+      });
+    });
+  });
+
   describe('#searchUris', function() {
     unroll(
       'returns the expected search URIs (#when)',