Skip to content
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

feat: ✨ Catfishes know about other catfishes #264

Open
wants to merge 13 commits into
base: development
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 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
6 changes: 4 additions & 2 deletions client/src/components/ChatRoom.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@
export let lobbyCode: string;
export let isStalker: boolean;
export let isSpectator: boolean;
export let catfishes: string[]; // empty for cats or spectators | at least one for catfishes
// variables
let user = $authStore as User;
let partner: string | undefined = undefined;
let partnerInfo: Player | undefined;
let pairInfo: [Player, Player] | undefined;
let chatRoomId: string | undefined = undefined;
Expand Down Expand Up @@ -51,7 +53,7 @@
pairInfo = chatRoom.pair.map((uid) => lobby.players[lobby.uids.indexOf(uid)]) as [Player, Player];
} else {
// Get partnerInfo
const partner = chatRoom.pair.find((uid) => user.uid !== uid);
partner = chatRoom.pair.find((uid) => user.uid !== uid);
if (partner !== undefined) {
partnerInfo = lobby.players[lobby.uids.indexOf(partner)];
}
Expand Down Expand Up @@ -116,7 +118,7 @@
>
<div slot="before-messages" class="matched-with mdc-typography--headline5">
{#if partnerInfo !== undefined}
You matched with {partnerInfo.displayName}
You matched with <span class:catfish={catfishes.includes(partner ?? "")}>{partnerInfo.displayName}</span>
{:else if pairInfo !== undefined}
{pairInfo[0].displayName} matched with {pairInfo[1].displayName}
{/if}
Expand Down
24 changes: 22 additions & 2 deletions client/src/components/ReceiveRole.svelte
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
<script lang="ts">
import FullScreenTransition from "./FullScreenTransition.svelte";
import type { PrivatePlayer, Role } from "$lib/firebase/firestore-types/lobby";
import type { Lobby, PrivatePlayer, Role } from "$lib/firebase/firestore-types/lobby";
import catImage from "$lib/images/role/cat.webp";
import catfishImage from "$lib/images/role/catfish.webp";

export let privatePlayer: PrivatePlayer;
export let catfishes: string[]; // empty for cats or spectators | at least 1 for catfishes
export let lobby: Lobby;

const formatter = new Intl.ListFormat("en", { style: "long", type: "conjunction" });

const ROLES: {
[key in Role]: { name: string; imageSrc: string; description: string };
Expand All @@ -26,13 +30,29 @@
},
};

function getCatfishNames() {
if (catfishes == undefined) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

is this if still needed?


const { uids, players } = lobby;

const catfishNames = catfishes.map((catfishId) => players[uids.indexOf(catfishId)].displayName);

return catfishNames;
}

$: role = ROLES[privatePlayer.role];
</script>

<FullScreenTransition imageSrc={role.imageSrc} imageAlt="">
<svelte:fragment slot="banner">Time to find your purrfect match!</svelte:fragment>
<svelte:fragment slot="image-subtext">
You are a <span class={role.name.toLowerCase()}>{role.name}</span>
{#if catfishes.length < 2}
You are a <span class={role.name.toLowerCase()}>{role.name}</span>
{:else}
{formatter.format(getCatfishNames() ?? "")} are the <span class={role.name.toLowerCase()}>{role.name}</span>
{/if}
</svelte:fragment>
<svelte:fragment slot="description">{role.description}</svelte:fragment>
</FullScreenTransition>
Expand Down
5 changes: 4 additions & 1 deletion client/src/components/Vote.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

export let lobby: Lobby;
export let lobbyCode: string;
export let catfishes: string[]; // empty for cats or spectators | at least one for catfishes

let errorMessage: string = "";

Expand Down Expand Up @@ -60,7 +61,9 @@
on:click={() => vote(lobby.uids[i])}
>
<AvatarImg {avatar} />
<span class="mdc-typography--subtitle1">{displayName ?? ""}</span>
<span class="mdc-typography--subtitle1 " class:catfish={catfishes.includes(lobby.uids[i])}
Copy link
Member

Choose a reason for hiding this comment

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

you acedentaly added space in the class

>{displayName ?? ""}</span
Copy link
Member

@nstringham nstringham Jan 10, 2023

Choose a reason for hiding this comment

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

personally I like this

<span>
  text
</span>

more than this

<span
>text</span
>

>
<div class="mdc-typography--caption">
{#if alive}
Answer: {promptAnswer ?? "no answer"}
Expand Down
41 changes: 37 additions & 4 deletions client/src/routes/game/+page.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import Header from "$components/Header.svelte";
import LobbySettings from "$components/LobbySettings.svelte";

import { onSnapshot, doc, getDoc, type Unsubscribe } from "firebase/firestore";
import { onSnapshot, doc, getDoc, type Unsubscribe, query, where, CollectionReference } from "firebase/firestore";
import { onMount, onDestroy } from "svelte";
import { getPrivatePlayerCollection, lobbyCollection } from "$lib/firebase/firestore-collections";
import { GAME_STATE_DURATIONS_DEFAULT, type Lobby, type PrivatePlayer } from "$lib/firebase/firestore-types/lobby";
Expand All @@ -29,6 +29,11 @@
let privatePlayer: PrivatePlayer | undefined = undefined;
let unsubscribePrivatePlayer: Unsubscribe | undefined = undefined;

let privatePlayerCollection: CollectionReference<PrivatePlayer>;

let catfishes: string[] = []; // empty for cats or spectators | at least one for catfishes
let unsubscribeCatfishes: Unsubscribe | undefined = undefined;

let countdown = GAME_STATE_DURATIONS_DEFAULT.WAIT;
$: countdownVisible = lobby != undefined && DISPLAY_TIMERS[lobby.state] == true;
let timer: ReturnType<typeof setInterval>;
Expand Down Expand Up @@ -72,7 +77,7 @@
return;
}

const privatePlayerCollection = getPrivatePlayerCollection(lobbyDocRef);
privatePlayerCollection = getPrivatePlayerCollection(lobbyDocRef);
const privatePlayerDocRef = doc(privatePlayerCollection, $user.uid);

updateCountdown();
Expand Down Expand Up @@ -115,6 +120,8 @@
unsubscribeLobby?.();
// unsub from privatePlayer
unsubscribePrivatePlayer?.();
// unsub from catfishes query
unsubscribeCatfishes?.();
});

function errorToJoin(errorMessage: string) {
Expand Down Expand Up @@ -156,6 +163,31 @@
}
}
}

$: privatePlayer, getCatfishes();
function getCatfishes() {
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we call unsubscribeCatfishes here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I understand but explain why?

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure the way the code is currently written we resubscribe every time the private player changes (each time there is a new prompt for example) but we never unsubscribe so that will put a lot of unwanted stress on the database.

Copy link
Member

Choose a reason for hiding this comment

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

why is unsubscribeCatfishes not called if the state is ROLE?

Copy link
Member Author

Choose a reason for hiding this comment

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

So you saying subscribe for that moment then unsubscribe? And a side note I think the getDocs function might be better since we're not using the newly updated documents to display to the user or anything

Copy link
Member

Choose a reason for hiding this comment

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

No I'm saying unsubscribe before you subscribe

Copy link
Member

Choose a reason for hiding this comment

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

Yeah getDocs makes sense to me

Copy link
Member Author

Choose a reason for hiding this comment

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

No I'm saying unsubscribe before you subscribe
Yeah, that makes sense. You want to make sure you unsubscribe from the previous subscription before you subscribe again in the Role phases case.

// if the current player is a catfish we want them to know all the other catfishes
if (privatePlayer !== undefined && privatePlayer.role == "CATFISH") {
const catfishQuery = query<PrivatePlayer>(privatePlayerCollection, where("role", "==", "CATFISH"));

// only allow subscribing during ROLE phase
if (lobby?.state == "ROLE") {
unsubscribeCatfishes = onSnapshot(
catfishQuery,
(queryCollection) => {
catfishes = queryCollection.docs.map((doc) => doc.id);
},
(err) => {
console.error(err);
errorMessage = err instanceof Error ? err.message : String(err);
}
);
} else {
// otherwise unsubscribe
unsubscribeCatfishes?.();
}
}
}
</script>

<svelte:window on:beforeunload={onbeforeunload} />
Expand Down Expand Up @@ -204,7 +236,7 @@
<CircularProgress indeterminate />
</div>
{:else if lobby.state === "ROLE"}
<Role {privatePlayer} />
<Role {lobby} {privatePlayer} {catfishes} />
{:else if lobby.state === "PROMPT"}
{#if lobby.alivePlayers.includes($user.uid)}
<Prompt prompt={privatePlayer.prompt} uid={$user.uid} {lobbyCode} />
Expand All @@ -213,11 +245,12 @@
<ChatRoom
{lobby}
{lobbyCode}
{catfishes}
isStalker={privatePlayer.stalker}
isSpectator={!lobby.alivePlayers.includes($user.uid)}
/>
{:else if lobby.state === "VOTE"}
<Vote {lobby} {lobbyCode} />
<Vote {lobby} {lobbyCode} {catfishes} />
{:else if lobby.state === "RESULT"}
<Result {lobby} />
{:else if lobby.state === "END"}
Expand Down
4 changes: 4 additions & 0 deletions client/src/theme/dark/_smui-theme.scss
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,7 @@ a:visited {
.error {
color: theme.$error;
}

.catfish {
color: #d32f2f;
}
Copy link
Member

Choose a reason for hiding this comment

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

this code is duplicated in ReceiveRole maybe it would be best to move all three role colors into the theme

4 changes: 4 additions & 0 deletions client/src/theme/light/_smui-theme.scss
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,7 @@ a:visited {
.error {
color: theme.$error;
}

.catfish {
color: #d32f2f;
}
10 changes: 10 additions & 0 deletions firestore.rules
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,17 @@ service cloud.firestore {
}

match /privatePlayers/{uid} {

function isCatfish(){
return get(/databases/$(database)/documents/lobbies/$(code)/privatePlayers/$(request.auth.uid)).data.role == "CATFISH";
}

function isCatfishResource(){
return resource.data.role == "CATFISH"
}

allow get: if request.auth.uid == uid;
allow list: if isCatfish() && isCatfishResource();
Copy link
Member

Choose a reason for hiding this comment

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

do we really want catfish to be able to read the entire privatePlayer document?

Copy link
Member Author

Choose a reason for hiding this comment

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

They're only allowed to read other catfish private player docs in which only holds the their role(which we want them to know), stalker(wouldn't matter if they know because they're working together), and prompt in which they would have the same prompt.

Copy link
Member

Choose a reason for hiding this comment

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

hmm I guess so but it feels wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

If you have a better idea lmk. We'd be only duplicating a little bit of data if you do have one

}

match /promptAnswers/{uid} {
Expand Down