feat: add logo on access form #1668
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughA new optional boolean prop, Changes
Sequence Diagram(s)sequenceDiagram
participant Page
participant DataFetcher
participant ViewComponent
participant AccessForm
Page->>DataFetcher: getStaticProps()
DataFetcher->>Page: returns props (logoOnAccessForm set by teamId)
Page->>ViewComponent: Renders with logoOnAccessForm prop
ViewComponent->>AccessForm: Passes logoOnAccessForm
AccessForm->>AccessForm: If logoOnAccessForm & brand.logo, render nav bar with logo
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
components/view/access-form/index.tsx (2)
106-106: Consider using optional chaining for more concise code.The static analysis tool suggests using optional chaining here for better readability.
- if (field.required && !data.customFields?.[field.identifier!]) { + if (field.required && !data.customFields?.[field.identifier ?? '']) {🧰 Tools
🪛 Biome (1.9.4)
[error] 106-106: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
119-121: Consider enhancing logo accessibility.For better accessibility, consider using a more specific alt text that includes the brand name when available.
<img src={brand.logo as string} - alt="Brand Logo" + alt={`${brand.name || 'Brand'} Logo`} className="h-16 w-auto object-contain" />pages/view/domains/[domain]/[slug]/d/[documentId].tsx (1)
271-271: Consider more maintainable team ID managementThe feature is enabled based on a hardcoded team ID check. While this works for now, consider moving team-specific configurations to a centralized configuration file or environment variables for better maintainability as more teams adopt this feature.
- logoOnAccessForm: teamId === "cm7nlkrhm0000qgh0nvyrrywr", + logoOnAccessForm: TEAMS_WITH_LOGO_ON_ACCESS_FORM.includes(teamId),Where
TEAMS_WITH_LOGO_ON_ACCESS_FORMwould be imported from a central configuration file.pages/view/[linkId]/index.tsx (1)
150-152: Consider extracting team ID logicThe same team ID conditions are duplicated in both the document and dataroom branches. Consider extracting this logic into a helper function or a constant to improve maintainability.
+ const TEAMS_WITH_LOGO_ON_ACCESS_FORM = ["cm7nlkrhm0000qgh0nvyrrywr", "clup33by90000oewh4rfvp2eg"]; + const hasLogoOnAccessForm = (teamId) => TEAMS_WITH_LOGO_ON_ACCESS_FORM.includes(teamId); // Then in DOCUMENT_LINK branch - logoOnAccessForm: - teamId === "cm7nlkrhm0000qgh0nvyrrywr" || - teamId === "clup33by90000oewh4rfvp2eg", + logoOnAccessForm: hasLogoOnAccessForm(teamId), // And in DATAROOM_LINK branch - logoOnAccessForm: - teamId === "cm7nlkrhm0000qgh0nvyrrywr" || - teamId === "clup33by90000oewh4rfvp2eg", + logoOnAccessForm: hasLogoOnAccessForm(teamId),Also applies to: 222-224
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (8)
components/view/access-form/index.tsx(4 hunks)components/view/dataroom/dataroom-document-view.tsx(3 hunks)components/view/dataroom/dataroom-view.tsx(4 hunks)components/view/document-view.tsx(3 hunks)pages/view/[linkId]/d/[documentId].tsx(5 hunks)pages/view/[linkId]/index.tsx(6 hunks)pages/view/domains/[domain]/[slug]/d/[documentId].tsx(5 hunks)pages/view/domains/[domain]/[slug]/index.tsx(6 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
components/view/access-form/index.tsx
[error] 106-106: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (19)
components/view/document-view.tsx (1)
65-65: Properly integrated new logoOnAccessForm feature!The logoOnAccessForm parameter and type definition have been correctly added to the DocumentView component and appropriately passed down to the AccessForm component when displaying the access form for protected documents.
Also applies to: 87-87, 267-267
components/view/dataroom/dataroom-document-view.tsx (1)
78-78: Consistent implementation of logoOnAccessForm prop!The logoOnAccessForm parameter and type definition have been correctly added to the DataroomDocumentView component and properly passed to the AccessForm component, maintaining consistency with other view components.
Also applies to: 98-98, 311-311
components/view/dataroom/dataroom-view.tsx (2)
58-58: Properly integrated logoOnAccessForm prop!The logoOnAccessForm parameter and type definition have been correctly added to the DataroomView component and appropriately passed to the AccessForm component.
Also applies to: 74-74, 252-252
282-288: Improved readability of viewerEmail prop!The formatting change for the viewerEmail prop improves code readability while maintaining the same functionality.
components/view/access-form/index.tsx (1)
45-45: Well-implemented logo navigation bar!The implementation of the optional logo navigation bar is clean and effective:
- Only displays when both
logoOnAccessFormis true and a brand logo exists- Uses brand colors with sensible defaults
- Properly sizes the logo with fixed height and auto width
- Adjusts container padding to accommodate the new element
Also applies to: 63-63, 103-103, 109-125, 127-127
pages/view/domains/[domain]/[slug]/d/[documentId].tsx (3)
46-46: Type definition update for logoOnAccessFormThe new
logoOnAccessFormboolean property has been correctly added to theDataroomDocumentPropstype definition, maintaining proper type safety for this feature.
57-57: Parameter propagation for logoOnAccessFormThe
logoOnAccessFormparameter is correctly added to the component function signature, consistent with the type definition update.
174-174: Prop forwarding to child componentThe
logoOnAccessFormprop is properly passed down to theDataroomDocumentViewcomponent, enabling the logo display functionality.pages/view/[linkId]/d/[documentId].tsx (4)
46-46: Type definition update for logoOnAccessFormThe
logoOnAccessFormboolean property has been correctly added to theDataroomDocumentPropstype definition, maintaining proper type safety.
57-57: Parameter propagation for logoOnAccessFormThe
logoOnAccessFormparameter is correctly added to the component function signature, maintaining consistency with the updated type definition.
170-170: Prop forwarding to child componentThe
logoOnAccessFormprop is properly passed down to theDataroomDocumentViewcomponent.
263-263:Details
❌ Incorrect review comment
Team ID inconsistency across files
This file only enables the logo feature for one team ID (
cm7nlkrhm0000qgh0nvyrrywr), while other files in this PR enable it for two team IDs. Ensure this difference is intentional.Consider checking if these team-based conditions should be consistent across files:
🏁 Script executed:
#!/bin/bash # Search for logoOnAccessForm conditions across files to verify consistency rg "logoOnAccessForm: teamId === " --type=tsxLength of output: 75
Let’s search all .tsx files for any
logoOnAccessFormoccurrences using a glob-based ripgrep search (falling back to grep if needed):#!/bin/bash echo "=== ripgrep results ===" rg -n "logoOnAccessForm" -g "*.tsx" || echo "rg search failed or no matches" echo echo "=== grep fallback ===" grep -R -n "logoOnAccessForm" . --include="*.tsx" || echo "grep search failed or no matches"
Let’s retry searching across all
.tsxfiles without relying on built-in filetype definitions:
🏁 Script executed:
#!/bin/bash echo "=== ripgrep results ===" rg -n "logoOnAccessForm" -g "*.tsx" || echo "No matches found with rg" echo echo "=== grep fallback results ===" grep -R -n "logoOnAccessForm" . --include="*.tsx" || echo "No matches found with grep"Length of output: 5351
Ignore team-ID consistency concern
I searched all.tsxfiles and found the onlyteamId === "cm7nlkrhm0000qgh0nvyrrywr"checks gatinglogoOnAccessFormin:
- pages/view/[linkId]/d/[documentId].tsx (line 263)
- pages/view/domains/[domain]/[slug]/d/[documentId].tsx (line 271)
All other pages and components consume the shared
logoOnAccessFormboolean and do not repeat any team-ID checks. There are no instances enabling the logo for a second team. You can safely disregard the original comment.Likely an incorrect or invalid review comment.
pages/view/domains/[domain]/[slug]/index.tsx (4)
194-196: Feature flag consistency across link typesGood job maintaining consistency by applying the same team ID conditions for both document and dataroom link types within this file.
221-221: Parameter and type updates for ViewPageThe
logoOnAccessFormparameter and type definition are correctly added to theViewPagecomponent.Also applies to: 241-241
365-365: Consistent prop forwarding to view componentsThe
logoOnAccessFormprop is properly passed down to both theDocumentViewandDataroomViewcomponents, ensuring consistent behavior across different view types.Also applies to: 446-446
122-124:Details
❓ Verification inconclusive
Multiple team IDs for logoOnAccessForm
The feature is enabled for two specific teams here, which differs from some other files in this PR. Ensure this difference is intentional based on your business requirements.
🏁 Script executed:
#!/bin/bash # Show all team IDs that get the logoOnAccessForm feature across the codebase rg "logoOnAccessForm:.*teamId" --type=tsx -A 3 -B 1Length of output: 81
🏁 Script executed:
#!/bin/bash # Find all occurrences of logoOnAccessForm in the codebase without filtering by file type rg "logoOnAccessForm" -n -A 3 -B 1Length of output: 11588
Verify inconsistent logoOnAccessForm team IDs
Across the codebase, the
logoOnAccessFormflag is enabled for two teams in some pages but only one team in others. Please confirm this discrepancy is intentional:• pages/view/domains/[domain]/[slug]/index.tsx
logoOnAccessForm enabled for:
- cm7nlkrhm0000qgh0nvyrrywr
- clup33by90000oewh4rfvp2eg• pages/view/[linkId]/index.tsx
logoOnAccessForm enabled for:
- cm7nlkrhm0000qgh0nvyrrywr
- clup33by90000oewh4rfvp2eg• pages/view/domains/[domain]/[slug]/d/[documentId].tsx
logoOnAccessForm enabled only for: cm7nlkrhm0000qgh0nvyrrywr• pages/view/[linkId]/d/[documentId].tsx
logoOnAccessForm enabled only for: cm7nlkrhm0000qgh0nvyrrywrEnsure the difference in team coverage aligns with your business requirements.
pages/view/[linkId]/index.tsx (3)
58-58: Interface update for ViewPagePropsThe
logoOnAccessFormboolean property has been correctly added to theViewPagePropsinterface.
250-250: Parameter update for ViewPageThe
logoOnAccessFormparameter is correctly added to the component function signature.
382-382: Consistent prop forwardingThe
logoOnAccessFormprop is properly passed down to both theDocumentViewandDataroomViewcomponents, ensuring consistent behavior across different view types.Also applies to: 462-462
Summary by CodeRabbit