feat: team logos #40
feat: team logos #40
Conversation
- Added TeamSwitcher and TeamSwitcherItem components for team management. - Integrated TeamSwitcher into BaseLayout, replacing the previous sidebar header. - Updated auto-imports.d.ts to include global declarations. - Enhanced BaseLayout by removing unused code related to team switching functionality.
- Introduced a new logo field in the GetUserTeamsResponseSchema and FPTeam document. - Updated the TeamSwitcher and TeamSwitcherItem components to display team logos. - Added TeamLogo component for handling logo display logic. - Enhanced user store to include logo information for teams.
- Changed props to use optional types for logoUrl and classNames. - Simplified avatar index calculation for consistent avatar mapping based on team name. - Updated template to bind classNames dynamically for better styling flexibility.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds optional team logo support: new FPTeam Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant FE as "CreateTeamDialog (Frontend)"
participant Uploader as "FileUploader (Frontend)"
participant API as "Backend API (create_team)"
participant DB as "Database / FPTeam"
User->>FE: enter team name + upload/logo action
FE->>Uploader: upload file (optional)
Uploader->>API: POST file -> store attachment
API->>DB: persist attachment record
DB-->>API: return attachment URL
API-->>Uploader: attachment URL
Uploader-->>FE: provide logo_url
User->>FE: submit create team
FE->>API: POST /create_team {team_name, logo_url}
API->>DB: create FPTeam row (logo set if provided)
DB-->>API: created team
API-->>FE: return team object (includes logo)
FE-->>User: update UI / close dialog
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/components/team/CreateTeamDialog.vue (1)
45-54:⚠️ Potential issue | 🟡 MinorForm state is not reset when the dialog closes.
After creating a team (or dismissing the dialog),
form.team_name,form.logo_url, andformErrorsretain their previous values. The next time the dialog opens, the user sees stale data.🐛 Proposed fix — reset on close
Add a reset helper and call it when the dialog closes:
+function resetForm() { + form.team_name = ""; + form.logo_url = undefined; + formErrors.value = ""; +} + function createTeam() { const result = formSchema.safeParse(form); if (!result.success) { formErrors.value = result.error.issues.map((issue) => issue.message).join(", "); return; } formErrors.value = ""; user.createTeam(form.team_name, form.logo_url); + resetForm(); model.value = false; }Also watch
modelto reset on external close:watch(model, (open) => { if (!open) resetForm(); });frontend/src/components/fields/Attachment.vue (1)
19-40:⚠️ Potential issue | 🔴 Critical
export typeinside<script setup>violates Vue standards — this must be fixed.Vue's
<script setup>does not allow any ES module exports, including type-only exports. While TypeScript may allow it to compile, it contradicts Vue's design principles. MoveFileTypeto a dedicated.tsfile (e.g.,@/types/file.ts) or use a separate<script>block.The current import in
CreateTeamDialog.vue(line 7) will break once this is corrected, so it must also be updated.
🤖 Fix all issues with AI agents
In `@frontend/src/components/team/TeamSwitcher.vue`:
- Around line 55-66: The template uses non-null assertions on
userStore.currentTeam in TeamLogo and TeamSwitcherItem which will throw if
currentTeam is null; update the TeamSwitcher.vue template to guard against null
by conditionally rendering those components only when userStore.currentTeam
exists (e.g., v-if="userStore.currentTeam") or provide safe fallbacks for
team_name and logo (e.g., using optional chaining or default strings) so
TeamLogo and TeamSwitcherItem receive valid props; locate usages of currentTeam
in the template (TeamLogo and TeamSwitcherItem) and wrap them with a null-check
before accessing team_name or logo.
- Around line 47-49: The template uses CreateTeamDialog and TeamLogo but they
aren't imported in the <script setup> block, causing runtime "not defined"
errors; add explicit imports for these components (e.g., import CreateTeamDialog
from "./CreateTeamDialog.vue" and import TeamLogo from "./TeamLogo.vue") inside
the <script setup> so CreateTeamDialog and TeamLogo are registered and available
to the template.
In `@frontend/src/components/team/TeamSwitcherItem.vue`:
- Around line 4-13: The prop type for logoUrl in TeamSwitcherItem.vue is
currently defined as string | null which causes a TS2322 when passing it to the
TeamLogo component that expects string | undefined; update the props definition
used by defineProps (the props type near the type props = { label: string;
logoUrl: ... } and the call defineProps<props>()) to use string | undefined for
logoUrl (or coerce/transform null to undefined before passing to the TeamLogo
component) so the TeamLogo :logo-url binding receives the expected string |
undefined type.
🧹 Nitpick comments (4)
forms_pro/api/user.py (1)
12-12: Add a default value for the optionallogofield.If
logois absent from the input dict (not justNone), Pydantic will raise aValidationErrorsince there's no default. Other optional fields in this codebase (e.g.,last_nameon Line 19) use= None.Proposed fix
- logo: str | None = Field(description="Logo of the team") + logo: str | None = Field(default=None, description="Logo of the team")forms_pro/api/team.py (1)
47-55: Update the docstring to document the newlogo_urlparameter.Minor nit: the
Argssection doesn't mentionlogo_url.Proposed fix
""" Create a new team Args: team_name: Name of the team + logo_url: Optional URL of the team logo Returns: FPTeam - The created team """frontend/src/components/team/CreateTeamDialog.vue (1)
56-60: Unnecessary indirection — bindform.team_namedirectly.The separate
teamNameref synced via a watcher is redundant. You can usev-model="form.team_name"on the input (line 136) and passform.team_nametoTeamLogodirectly. This removes 5 lines and a watcher allocation.♻️ Proposed simplification
Remove lines 56-60 and update the template:
-const teamName = ref<string>(""); - -watch(teamName, (newVal) => { - form.team_name = newVal; -});In the template:
<TeamLogo - :team-name="teamName" + :team-name="form.team_name" class="size-12" :logo-url="form.logo_url" /> ... <input type="text" - v-model="teamName" + v-model="form.team_name" class="text-lg text-ink-gray-9 !outline-0 !ring-0 !border-0 bg-inherit text-center" placeholder="Enter team name" />frontend/src/layouts/BaseLayout.vue (1)
63-68:sidebarHeaderprop andSidebarHeaderPropstype are now unused.Since the header is now rendered via the
#headerslot withTeamSwitcher, thesidebarHeaderprop (lines 66-68) andSidebarHeaderPropstype (line 63) are dead code.
| <TeamLogo | ||
| v-if="isSidebarCollapsed" | ||
| :teamName="userStore.currentTeam!.team_name" | ||
| :logoUrl="userStore.currentTeam!.logo" | ||
| /> | ||
| <div v-else class="flex items-center gap-2 justify-between w-full"> | ||
| <TeamSwitcherItem | ||
| :label="userStore.currentTeam!.team_name" | ||
| :logo-url="userStore.currentTeam!.logo" | ||
| /> | ||
| <ChevronsUpDown class="size-4 text-ink-gray-6" /> | ||
| </div> |
There was a problem hiding this comment.
Non-null assertion on currentTeam will crash if the user has no teams.
Lines 57-58 and 62-63 use userStore.currentTeam! which throws a runtime error if currentTeam is null. This can happen before teams are loaded or if the user hasn't joined any team yet. Add a guard or fallback.
🐛 Proposed fix — guard against null
+ <template v-if="userStore.currentTeam">
<TeamLogo
v-if="isSidebarCollapsed"
- :teamName="userStore.currentTeam!.team_name"
- :logoUrl="userStore.currentTeam!.logo"
+ :teamName="userStore.currentTeam.team_name"
+ :logoUrl="userStore.currentTeam.logo"
/>
<div v-else class="flex items-center gap-2 justify-between w-full">
<TeamSwitcherItem
- :label="userStore.currentTeam!.team_name"
- :logo-url="userStore.currentTeam!.logo"
+ :label="userStore.currentTeam.team_name"
+ :logo-url="userStore.currentTeam.logo"
/>
<ChevronsUpDown class="size-4 text-ink-gray-6" />
</div>
+ </template>📝 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.
| <TeamLogo | |
| v-if="isSidebarCollapsed" | |
| :teamName="userStore.currentTeam!.team_name" | |
| :logoUrl="userStore.currentTeam!.logo" | |
| /> | |
| <div v-else class="flex items-center gap-2 justify-between w-full"> | |
| <TeamSwitcherItem | |
| :label="userStore.currentTeam!.team_name" | |
| :logo-url="userStore.currentTeam!.logo" | |
| /> | |
| <ChevronsUpDown class="size-4 text-ink-gray-6" /> | |
| </div> | |
| <template v-if="userStore.currentTeam"> | |
| <TeamLogo | |
| v-if="isSidebarCollapsed" | |
| :teamName="userStore.currentTeam.team_name" | |
| :logoUrl="userStore.currentTeam.logo" | |
| /> | |
| <div v-else class="flex items-center gap-2 justify-between w-full"> | |
| <TeamSwitcherItem | |
| :label="userStore.currentTeam.team_name" | |
| :logo-url="userStore.currentTeam.logo" | |
| /> | |
| <ChevronsUpDown class="size-4 text-ink-gray-6" /> | |
| </div> | |
| </template> |
🤖 Prompt for AI Agents
In `@frontend/src/components/team/TeamSwitcher.vue` around lines 55 - 66, The
template uses non-null assertions on userStore.currentTeam in TeamLogo and
TeamSwitcherItem which will throw if currentTeam is null; update the
TeamSwitcher.vue template to guard against null by conditionally rendering those
components only when userStore.currentTeam exists (e.g.,
v-if="userStore.currentTeam") or provide safe fallbacks for team_name and logo
(e.g., using optional chaining or default strings) so TeamLogo and
TeamSwitcherItem receive valid props; locate usages of currentTeam in the
template (TeamLogo and TeamSwitcherItem) and wrap them with a null-check before
accessing team_name or logo.
| type props = { | ||
| label: string; | ||
| logoUrl: string | null; | ||
| }; | ||
|
|
||
| const props = defineProps<props>(); | ||
| </script> | ||
| <template> | ||
| <div class="flex items-center gap-2 shrink-0"> | ||
| <TeamLogo :team-name="label" :logo-url="logoUrl" /> |
There was a problem hiding this comment.
Fix TypeScript error: string | null is not assignable to string | undefined.
The CI pipeline reports TS2322 on Line 13. The logoUrl prop is typed as string | null, but TeamLogo's logo-url prop likely expects string | undefined.
Proposed fix
- <TeamLogo :team-name="label" :logo-url="logoUrl" />
+ <TeamLogo :team-name="label" :logo-url="logoUrl ?? undefined" />Alternatively, align the prop type with TeamLogo's expectation by changing string | null to string | undefined on Line 6, if the parent components can accommodate that.
📝 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.
| type props = { | |
| label: string; | |
| logoUrl: string | null; | |
| }; | |
| const props = defineProps<props>(); | |
| </script> | |
| <template> | |
| <div class="flex items-center gap-2 shrink-0"> | |
| <TeamLogo :team-name="label" :logo-url="logoUrl" /> | |
| type props = { | |
| label: string; | |
| logoUrl: string | null; | |
| }; | |
| const props = defineProps<props>(); | |
| </script> | |
| <template> | |
| <div class="flex items-center gap-2 shrink-0"> | |
| <TeamLogo :team-name="label" :logo-url="logoUrl ?? undefined" /> |
🧰 Tools
🪛 GitHub Actions: Frontend TypeScript
[error] 13-13: TS2322: Type 'string | null' is not assignable to type 'string | undefined'.
🤖 Prompt for AI Agents
In `@frontend/src/components/team/TeamSwitcherItem.vue` around lines 4 - 13, The
prop type for logoUrl in TeamSwitcherItem.vue is currently defined as string |
null which causes a TS2322 when passing it to the TeamLogo component that
expects string | undefined; update the props definition used by defineProps (the
props type near the type props = { label: string; logoUrl: ... } and the call
defineProps<props>()) to use string | undefined for logoUrl (or coerce/transform
null to undefined before passing to the TeamLogo component) so the TeamLogo
:logo-url binding receives the expected string | undefined type.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/team/CreateTeamDialog.vue (1)
45-54:⚠️ Potential issue | 🟡 MinorForm state should be reset when the dialog closes.
The
createTeamcall does have error handling via toast notifications, but the form state (form.team_name,form.logo_url,teamName) is never reset. When the user opens the dialog again, the previous entry is still visible, which is a minor UX inconsistency. Reset the form fields either when the dialog closes or on successful team creation.
🤖 Fix all issues with AI agents
In `@frontend/src/components/team/CreateTeamDialog.vue`:
- Around line 56-60: Remove the unnecessary teamName ref and its watch: delete
the declaration of teamName and the watch(...) that assigns form.team_name, then
update the template bindings to use form.team_name directly (use
v-model="form.team_name" on the input and :team-name="form.team_name" on the
TeamLogo component) so the reactive form object is the single source of truth
and avoids potential desync between teamName and form.team_name.
🧹 Nitpick comments (2)
frontend/src/components/team/CreateTeamDialog.vue (2)
101-130: Minor: simplify event handler and trim unused slot bindings.
@success="(file: FileType) => setTeamLogo(file)"can be shortened to@success="setTeamLogo". The large destructured slot scope only useserror,uploading, andopenFileSelector— the rest (file,progress,uploaded,message,total,success) are dead bindings. Keeping only the three you need improves readability and removes the need for the@vue-ignorecomment.
141-148: Button is enabled on initial render despite empty (invalid) form.
formErrorsstarts as""and the watcher only fires on change, so the "Create Team" button is clickable before the user types anything.createTeam()does guard with its own validation, so this is safe — but for a polished UX you could either run an initial validation pass or derive the disabled state from the form content (e.g.,form.team_name.length < 2).
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores