refactor: partial state set actions instead of prioritised actions

gmus-backend/pkg/server/actions.go

@@ -20,7 +20,6 @@ const (
 type Action struct {
 	Type       ActionType  `json:"type"`
 	FromClient *string     `json:"fromClient"`
-	Priority   *int        `json:"priority"`
 	Payload    interface{} `json:"payload"`
 }
 

gmus-backend/pkg/server/clients.go

@@ -101,7 +101,7 @@ func (c *Client) subscribeToMe(l *logger.Logger, rdb redis.Cmdable) {
 			actionFromClient.FromClient = &c.name
 
 			if err := PublishActionFromClient(rdb, &actionFromClient); err != nil {
-				l.Error("Error publishing action from client: %v\n", err)
+				l.Error("Error publishing action from client (%v): %v\n", actionFromClient.Payload, err)
 			}
 		}
 	}

gmus-backend/pkg/server/types.go

@@ -44,12 +44,12 @@ type Member struct {
 // Each client implementation MUST adhere to this spec.
 
 type MusicPlayer struct {
-	SongId        *int      `json:"songId" validate:"omitempty,gte=1"`
-	Playing       bool      `json:"playing" validate:"-"`
-	CurrentTime   float32   `json:"currentTime" validate:"gte=0"`
-	SeekTime      float32   `json:"seekTime" validate:"min=-1"`
-	Master        string    `json:"master" validate:"required"`
-	ActiveClients *[]string `json:"activeClients" validate:"required"`
-	Queue         *[]int    `json:"queue" validate:"required"`
-	ShuffleMode   bool      `json:"shuffleMode" validate:"-"`
+	SongId        *int      `json:"songId,omitempty" validate:"omitempty,gte=1"`
+	Playing       *bool     `json:"playing,omitempty" validate:"-"`
+	CurrentTime   *float32  `json:"currentTime,omitempty" validate:"omitempty,gte=0"`
+	SeekTime      *float32  `json:"seekTime,omitempty" validate:"omitempty,min=-1"`
+	Master        string    `json:"master,omitempty"`
+	ActiveClients *[]string `json:"activeClients,omitempty"`
+	Queue         *[]int    `json:"queue,omitempty"`
+	ShuffleMode   *bool     `json:"shuffleMode,omitempty" validate:"-"`
 }

gmus-mobile/lib/controller.dart

@@ -94,7 +94,6 @@ class Controller extends GetxController {
     if (!this._isMaster()) {
       this.socket.dispatch(jsonEncode({
         'type': actions.STATE_SET,
-        'priority': 0,
         'payload': this.player.value.stringify(),
       }));
     }
@@ -112,7 +111,6 @@ class Controller extends GetxController {
 
     this.socket.dispatch(jsonEncode({
       'type': actions.STATE_SET,
-      'priority': 0,
       'payload': this.player.value.stringify(),
     }));
   }

gmus-mobile/test/controller_test.dart

@@ -69,7 +69,7 @@ void main() {
 
             controller.playPause();
 
-            verify(controller.socket.channel.sink.add('{"type":"STATE_SET","priority":0,"payload":{"songId":null,"playing":true,"currentTime":0.0,"seekTime":-1.0,"master":"other-client-name-master","activeClients":[],"queue":[],"shuffleMode":false}}')).called(1);
+            verify(controller.socket.channel.sink.add('{"type":"STATE_SET","payload":{"songId":null,"playing":true,"currentTime":0.0,"seekTime":-1.0,"master":"other-client-name-master","activeClients":[],"queue":[],"shuffleMode":false}}')).called(1);
           });
         });
 
@@ -81,7 +81,7 @@ void main() {
 
             controller.playPause();
 
-            verify(controller.socket.channel.sink.add('{"type":"STATE_SET","priority":0,"payload":{"songId":182,"playing":false,"currentTime":0.0,"seekTime":-1.0,"master":"other-client-name-master","activeClients":[],"queue":[],"shuffleMode":false}}')).called(1);
+            verify(controller.socket.channel.sink.add('{"type":"STATE_SET","payload":{"songId":182,"playing":false,"currentTime":0.0,"seekTime":-1.0,"master":"other-client-name-master","activeClients":[],"queue":[],"shuffleMode":false}}')).called(1);
           });
         });
       });
@@ -92,7 +92,7 @@ void main() {
           controller.player.value.playing = false;
           controller.playPause();
 
-          verifyNever(controller.socket.channel.sink.add('{"type":"STATE_SET","priority":0,"payload":{"songId":null,"playing":true,"currentTime":0.0,"seekTime":-1.0,"master":"other-client-name-master","activeClients":[],"queue":[],"shuffleMode":false}}'));
+          verifyNever(controller.socket.channel.sink.add('{"type":"STATE_SET","payload":{"songId":null,"playing":true,"currentTime":0.0,"seekTime":-1.0,"master":"other-client-name-master","activeClients":[],"queue":[],"shuffleMode":false}}'));
         });
       });
     });
@@ -104,7 +104,7 @@ void main() {
 
           controller.playSong(871);
 
-          verify(controller.socket.channel.sink.add('{"type":"STATE_SET","priority":0,"payload":{"songId":871,"playing":true,"currentTime":0.0,"seekTime":-1.0,"master":"other-client-name-master","activeClients":[],"queue":[],"shuffleMode":false}}')).called(1);
+          verify(controller.socket.channel.sink.add('{"type":"STATE_SET","payload":{"songId":871,"playing":true,"currentTime":0.0,"seekTime":-1.0,"master":"other-client-name-master","activeClients":[],"queue":[],"shuffleMode":false}}')).called(1);
         });
       });
 

gmus-web/src/actions/actions.ts

@@ -3,17 +3,14 @@ import type { SetStateAction } from 'react';
 import type { Song } from '../types';
 import type { Member, MusicPlayer } from '../types/state';
 import type { ActionErrorOccurred } from './error';
-import {
-  ActionLocal,
-  ActionRemote,
-  ActionTypeLocal,
-  ActionTypeRemote,
-  LocalStateSetPayload,
-} from './types';
+import { ActionLocal, ActionRemote, ActionTypeLocal, ActionTypeRemote } from './types';
 
 export * from './types';
 
-export type ActionStateSetRemote = ActionRemote<ActionTypeRemote.StateSet, MusicPlayer | null>;
+export type ActionStateSetRemote = ActionRemote<
+  ActionTypeRemote.StateSet,
+  Partial<MusicPlayer> | null
+>;
 
 export type ActionClientListUpdated = ActionRemote<ActionTypeRemote.ClientListUpdated, Member[]>;
 
@@ -29,14 +26,16 @@ export const nameSet = (name: string): ActionNameSet => ({
   payload: name,
 });
 
-export type ActionStateSetLocal = ActionLocal<ActionTypeLocal.StateSet, LocalStateSetPayload>;
+export type ActionStateSetLocal = ActionLocal<
+  ActionTypeLocal.StateSet,
+  SetStateAction<Partial<MusicPlayer>>
+>;
 
 export const stateSet = (
   payload: SetStateAction<Partial<MusicPlayer>> = {},
-  priority = 0,
 ): ActionStateSetLocal => ({
   type: ActionTypeLocal.StateSet,
-  payload: { payload, priority },
+  payload,
 });
 
 export type ActionSeeked = ActionLocal<ActionTypeLocal.Seeked, number>;

gmus-web/src/actions/types.ts

@@ -1,7 +1,3 @@
-import type { SetStateAction } from 'react';
-
-import type { MusicPlayer } from '../types';
-
 // Remote actions - these only come FROM the socket
 export enum ActionTypeRemote {
   StateSet = 'STATE_SET',
@@ -33,11 +29,6 @@ export interface Action<T extends string = string, P = unknown> {
 export type ActionRemote<T extends ActionTypeRemote = ActionTypeRemote, P = unknown> = Action<
   T,
   P
-> & { priority: number; fromClient?: string | null };
+> & { fromClient?: string | null };
 
 export type ActionLocal<T extends ActionTypeLocal = ActionTypeLocal, P = unknown> = Action<T, P>;
-
-export type LocalStateSetPayload = {
-  payload: SetStateAction<Partial<MusicPlayer>>;
-  priority?: number;
-};

gmus-web/src/components/app.tsx

@@ -45,7 +45,7 @@ export const App: React.FC<Props> = ({
 
   const onTimeUpdate = useCallback(
     (currentTime: number): void => {
-      dispatch(stateSet({ currentTime }, 1));
+      dispatch(stateSet({ currentTime }));
     },
     [dispatch],
   );

gmus-web/src/effects/effects.spec.ts

@@ -38,16 +38,28 @@ describe(globalEffects.name, () => {
         shuffleMode: false,
       };
 
-      const action = stateSet(localPlayer, 3);
+      const action = stateSet(localPlayer);
 
       const result = globalEffects(state, action);
 
       expect(result).toStrictEqual<ActionStateSetRemote>({
         type: ActionTypeRemote.StateSet,
-        priority: 3,
         payload: { ...state.player, ...localPlayer },
       });
     });
+
+    describe('when the payload is a partial', () => {
+      const actionPartial = stateSet({ master: 'my-client-name', currentTime: 143 });
+
+      it('should create a partial remote state set action', () => {
+        expect.assertions(1);
+        const result = globalEffects(state, actionPartial);
+        expect(result).toStrictEqual<ActionStateSetRemote>({
+          type: ActionTypeRemote.StateSet,
+          payload: { master: 'my-client-name', currentTime: 143 },
+        });
+      });
+    });
   });
 
   describe(ActionTypeLocal.Seeked, () => {
@@ -86,8 +98,7 @@ describe(globalEffects.name, () => {
 
         expect(result).toStrictEqual<ActionStateSetRemote>({
           type: ActionTypeRemote.StateSet,
-          priority: 0,
-          payload: { ...state.player, seekTime: 776 },
+          payload: { seekTime: 776 },
         });
       });
     });
@@ -112,13 +123,12 @@ describe(globalEffects.name, () => {
 
     const action = masterSet();
 
-    it('should return a StateSet action informing other clients that we are the new master', () => {
+    it('should return a full StateSet action informing other clients that we are the new master', () => {
       expect.assertions(1);
       const result = globalEffects(stateMasterWentAway, action);
 
       expect(result).toStrictEqual<ActionStateSetRemote>({
         type: ActionTypeRemote.StateSet,
-        priority: 0,
         payload: {
           songId: 123,
           playing: false,
@@ -133,13 +143,12 @@ describe(globalEffects.name, () => {
     });
 
     describe('when the action specified a particular client', () => {
-      it('should return a StateSet action informing the new client to resume playback', () => {
+      it('should return a full StateSet action informing the new client to take over master status', () => {
         expect.assertions(1);
         const result = globalEffects(stateMasterWentAway, masterSet('other-client'));
 
         expect(result).toStrictEqual<ActionStateSetRemote>({
           type: ActionTypeRemote.StateSet,
-          priority: 0,
           payload: {
             songId: 123,
             playing: true,
@@ -173,10 +182,9 @@ describe(globalEffects.name, () => {
 
         expect(result).toStrictEqual<ActionStateSetRemote>({
           type: ActionTypeRemote.StateSet,
-          priority: 0,
-          payload: expect.objectContaining({
+          payload: {
             activeClients: ['other-client'],
-          }),
+          },
         });
       });
     });
@@ -196,10 +204,9 @@ describe(globalEffects.name, () => {
 
         expect(result).toStrictEqual<ActionStateSetRemote>({
           type: ActionTypeRemote.StateSet,
-          priority: 0,
-          payload: expect.objectContaining({
+          payload: {
             activeClients: expect.arrayContaining(['some-client', 'other-client']),
-          }),
+          },
         });
       });
     });
@@ -234,16 +241,8 @@ describe(globalEffects.name, () => {
 
         expect(result).toStrictEqual<ActionStateSetRemote>({
           type: ActionTypeRemote.StateSet,
-          priority: 0,
           payload: {
-            songId: 123,
             playing: false,
-            currentTime: 83,
-            seekTime: 5,
-            master: 'some-master-client',
-            activeClients: [],
-            queue: [],
-            shuffleMode: false,
           },
         });
       });
@@ -287,16 +286,11 @@ describe(globalEffects.name, () => {
 
         expect(result).toStrictEqual<ActionStateSetRemote>({
           type: ActionTypeRemote.StateSet,
-          priority: 0,
           payload: {
             songId: 185,
             playing: true,
             currentTime: 0,
             seekTime: 0,
-            master: 'some-master-client',
-            activeClients: [],
-            queue: [],
-            shuffleMode: false,
           },
         });
       });
@@ -316,21 +310,17 @@ describe(globalEffects.name, () => {
   describe(ActionTypeLocal.QueuePushed, () => {
     const action = queuePushed([184, 79]);
 
+    const stateWithQueue: GlobalState = {
+      ...initialState,
+      player: { ...initialState.player, master: 'some-master', queue: [23] },
+    };
+
     it('should add to the end of the queue', () => {
       expect.assertions(1);
-      const result = globalEffects(
-        {
-          ...initialState,
-          player: { ...initialState.player, master: 'some-master', queue: [23] },
-        },
-        action,
-      );
+      const result = globalEffects(stateWithQueue, action);
       expect(result).toStrictEqual<ActionStateSetRemote>({
         type: ActionTypeRemote.StateSet,
-        priority: 0,
         payload: {
-          ...initialState.player,
-          master: 'some-master',
           queue: [23, 184, 79],
         },
       });
@@ -363,10 +353,7 @@ describe(globalEffects.name, () => {
       const result = globalEffects(stateWithQueue, action);
       expect(result).toStrictEqual<ActionStateSetRemote>({
         type: ActionTypeRemote.StateSet,
-        priority: 0,
         payload: {
-          ...initialState.player,
-          master: 'some-master',
           playing: true,
           songId: 8843,
           currentTime: 0,
@@ -392,10 +379,7 @@ describe(globalEffects.name, () => {
 
       expect(result).toStrictEqual<ActionStateSetRemote>({
         type: ActionTypeRemote.StateSet,
-        priority: 0,
         payload: {
-          ...initialState.player,
-          master: 'some-master',
           queue: [17, 23],
         },
       });
@@ -421,10 +405,7 @@ describe(globalEffects.name, () => {
 
       expect(result).toStrictEqual<ActionStateSetRemote>({
         type: ActionTypeRemote.StateSet,
-        priority: 0,
         payload: {
-          ...initialState.player,
-          master: 'some-master',
           queue: expectedResult,
         },
       });

gmus-web/src/effects/effects.ts

@@ -33,9 +33,7 @@ function pushToQueue(state: GlobalState, action: ActionQueuePushed): RemoteActio
   }
   return {
     type: ActionTypeRemote.StateSet,
-    priority: 0,
     payload: {
-      ...state.player,
       queue: nextQueue,
     },
   };
@@ -45,14 +43,13 @@ function sendStateUpdateToServer(
   state: GlobalState,
   action: ActionStateSetLocal,
 ): RemoteAction | null {
-  const nextPlayer = getNextPlayerStateFromAction(state.player, action.payload);
+  const nextPlayer = getNextPlayerStateFromAction(state.player, action);
   if (!state.player.master && !nextPlayer?.master) {
     return null;
   }
   return {
     type: ActionTypeRemote.StateSet,
-    priority: action.payload.priority ?? 0,
-    payload: nextPlayer,
+    payload: typeof action.payload === 'function' ? action.payload(state.player) : action.payload,
   };
 }
 
@@ -67,15 +64,13 @@ export function globalEffects(state: GlobalState, action: LocalAction): RemoteAc
       }
       return {
         type: ActionTypeRemote.StateSet,
-        priority: 0,
-        payload: { ...state.player, seekTime: action.payload },
+        payload: { seekTime: action.payload },
       };
 
     case ActionTypeLocal.MasterSet:
       if (action.payload) {
         return {
           type: ActionTypeRemote.StateSet,
-          priority: 0,
           payload: {
             ...state.player,
             seekTime: state.player.currentTime,
@@ -86,7 +81,6 @@ export function globalEffects(state: GlobalState, action: LocalAction): RemoteAc
 
       return {
         type: ActionTypeRemote.StateSet,
-        priority: 0,
         payload: {
           ...state.player,
           playing: false,
@@ -98,9 +92,7 @@ export function globalEffects(state: GlobalState, action: LocalAction): RemoteAc
     case ActionTypeLocal.ActiveClientToggled:
       return {
         type: ActionTypeRemote.StateSet,
-        priority: 0,
         payload: {
-          ...state.player,
           activeClients: state.player.activeClients.includes(action.payload)
             ? state.player.activeClients.filter((client) => client !== action.payload)
             : [...state.player.activeClients, action.payload],
@@ -110,9 +102,7 @@ export function globalEffects(state: GlobalState, action: LocalAction): RemoteAc
     case ActionTypeLocal.PlayPaused:
       return {
         type: ActionTypeRemote.StateSet,
-        priority: 0,
         payload: {
-          ...state.player,
           playing: !state.player.playing,
         },
       };
@@ -123,9 +113,7 @@ export function globalEffects(state: GlobalState, action: LocalAction): RemoteAc
       }
       return {
         type: ActionTypeRemote.StateSet,
-        priority: 0,
         payload: {
-          ...state.player,
           songId: action.payload.song?.id ?? null,
           playing: !!action.payload.song,
           currentTime: 0,
@@ -141,9 +129,7 @@ export function globalEffects(state: GlobalState, action: LocalAction): RemoteAc
       }
       return {
         type: ActionTypeRemote.StateSet,
-        priority: 0,
         payload: {
-          ...state.player,
           queue: state.player.queue.slice(1),
           playing: !!state.player.queue[0],
           songId: state.player.queue[0],
@@ -157,17 +143,14 @@ export function globalEffects(state: GlobalState, action: LocalAction): RemoteAc
       }
       return {
         type: ActionTypeRemote.StateSet,
-        priority: 0,
         payload: {
-          ...state.player,
           queue: state.player.queue.filter((id) => id !== action.payload),
         },
       };
     case ActionTypeLocal.QueueOrdered:
       return {
         type: ActionTypeRemote.StateSet,
-        priority: 0,
-        payload: { ...state.player, queue: reorderQueue(state.player.queue, action) },
+        payload: { queue: reorderQueue(state.player.queue, action) },
       };
 
     default:

gmus-web/src/reducer/reducer.spec.ts

@@ -33,7 +33,6 @@ describe(globalReducer.name, () => {
         const actionFromOtherClient: ActionStateSetRemote = {
           type: ActionTypeRemote.StateSet,
           fromClient: 'other-client',
-          priority: 0,
           payload: {
             songId: 123,
             playing: true,
@@ -62,19 +61,20 @@ describe(globalReducer.name, () => {
           });
         });
 
-        describe('when the priority was 1', () => {
-          const actionWithLowerPriority: ActionStateSetRemote = {
-            ...actionFromOtherClient,
-            priority: 1,
+        describe('when the payload is partial', () => {
+          const actionPartial: ActionStateSetRemote = {
+            type: ActionTypeRemote.StateSet,
+            payload: { currentTime: 102, playing: true },
           };
 
-          it('should only update the currentTime', () => {
+          it('should only update the given properties', () => {
             expect.assertions(1);
-            const result = globalReducer(stateMaster, actionWithLowerPriority);
+            const result = globalReducer(stateMaster, actionPartial);
 
             expect(result.player).toStrictEqual<MusicPlayer>({
               ...stateMaster.player,
-              currentTime: 75,
+              currentTime: 102,
+              playing: true,
             });
           });
         });

gmus-web/src/reducer/reducer.ts

@@ -37,20 +37,8 @@ function shouldSetSeekTime(state: GlobalState, action: ActionStateSetRemote): bo
   return willBeMaster(state, action) && !(isMaster(state) && isFromOurselves(state, action));
 }
 
-function getNextRemotePlayer(
-  existingPlayer: MusicPlayer,
-  action: ActionStateSetRemote,
-): MusicPlayer {
-  if (!action.payload) {
-    return action.priority > 0 ? existingPlayer : nullPlayer;
-  }
-  return action.priority > 0
-    ? { ...existingPlayer, currentTime: action.payload.currentTime }
-    : action.payload;
-}
-
 function onRemoteStateSet(state: GlobalState, action: ActionStateSetRemote): GlobalState {
-  const nextPlayer = getNextRemotePlayer(state.player, action);
+  const nextPlayer = action.payload ? { ...state.player, ...action.payload } : nullPlayer;
   const seekTime = shouldSetSeekTime(state, action) ? nextPlayer.seekTime : -1;
 
   const nextPlayerWithSeekTime: MusicPlayer = { ...nextPlayer, seekTime };
@@ -59,7 +47,7 @@ function onRemoteStateSet(state: GlobalState, action: ActionStateSetRemote): Glo
 }
 
 function onLocalStateSet(state: GlobalState, action: ActionStateSetLocal): GlobalState {
-  const nextPlayer = getNextPlayerStateFromAction(state.player, action.payload);
+  const nextPlayer = getNextPlayerStateFromAction(state.player, action);
   if (!nextPlayer) {
     return state;
   }

gmus-web/src/selectors.ts

@@ -4,16 +4,15 @@ import { MusicPlayer } from './types';
 
 export function getNextPlayerStateFromAction(
   player: MusicPlayer | undefined,
-  action: ActionStateSetLocal['payload'] | ActionStateSetRemote | null,
+  action: ActionStateSetLocal | ActionStateSetRemote | null,
 ): MusicPlayer | null {
   if (!(action && player)) {
     return null;
   }
-  const { payload } = action;
-  if (typeof payload === 'function') {
-    return { ...player, ...payload(player) };
+  if (typeof action.payload === 'function') {
+    return { ...player, ...action.payload(player) };
   }
-  return { ...player, ...payload };
+  return { ...player, ...action.payload };
 }
 
 export const isMaster = (state: Pick<GlobalState, 'player' | 'myClientName'>): boolean =>
@@ -27,18 +26,10 @@ export const isFromOurselves = (
   action: ActionRemote,
 ): boolean => state.myClientName === action.fromClient;
 
-const isLocalStateSet = (
-  action: ActionStateSetLocal | ActionStateSetRemote,
-): action is ActionStateSetLocal => Reflect.has(action.payload ?? {}, 'priority');
-
 export const willBeMaster = (
   state: Partial<GlobalState> & Pick<GlobalState, 'myClientName'>,
-  fullAction: ActionStateSetLocal | ActionStateSetRemote,
+  action: ActionStateSetLocal | ActionStateSetRemote,
 ): boolean => {
-  const action: ActionStateSetLocal['payload'] | ActionStateSetRemote = isLocalStateSet(fullAction)
-    ? fullAction.payload
-    : fullAction;
-
   const actionHasMaster =
     typeof action.payload === 'function' ? !!action.payload({}).master : !!action.payload?.master;
   return (