[Bug] (SC-204212) Improve site name fallback handling in Filters component#66
Open
[Bug] (SC-204212) Improve site name fallback handling in Filters component#66
Filters component#66Conversation
Reviewer's GuideReplaces the generic getOptions-based site selector in the Filters component with explicit site option construction that prefers brandingName, then name, then a predictable "Unnamed Site (id)" fallback, and adds tests to verify the new fallback behavior. Class diagram for updated Filters component props and option constructionclassDiagram
class Filters {
+Filters(props Props)
}
class Props {
+Maybe~Array~Site~~ sites
+Maybe~string~ siteId
+boolean isFetching
+function onChangeSearchQuery
+function onChangeSite
}
class Site {
+string id
+string brandingName
+string name
}
class Option {
+string value
+string label
+string key
+string type
}
Props --> "*" Site : uses
Filters --> Props : receives
Filters --> "*" Option : creates siteOptions
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
siteOptionsarray is rebuilt on every render whereas the previous implementation useduseMemo; consider wrapping thesites.map(...)inuseMemoagain ifsitescan be large or the component re-renders frequently.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `siteOptions` array is rebuilt on every render whereas the previous implementation used `useMemo`; consider wrapping the `sites.map(...)` in `useMemo` again if `sites` can be large or the component re-renders frequently.
## Individual Comments
### Comment 1
<location> `src/components/LinkDevice/blocks/Filters.tsx:16-20` </location>
<code_context>
const Filters: FC<Props> = ({ sites, siteId, isFetching, onChangeSearchQuery, onChangeSite }) => {
- const options = useMemo(() => getOptions(sites, "brandingName"), [sites]) as Array<Option>;
+ const siteOptions: Option[] = sites.map((site) => ({
+ value: site.id,
+ label: site.brandingName || site.name || `Unnamed Site (${site.id})`,
+ key: site.id,
+ type: "value",
+ }))
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle `sites` being null/undefined before calling `.map`.
Since `sites` is `Maybe<Site[]>`, it can be `null`/`undefined`, so `sites.map` risks a runtime error. Normalize it first, e.g. `const siteOptions: Option[] = (sites ?? []).map(...)`, so the component still renders safely when no sites are provided.
</issue_to_address>
### Comment 2
<location> `src/components/LinkDevice/blocks/__tests__/Filters.test.tsx:23-32` </location>
<code_context>
+
+
+describe("Filters", () => {
+ it.each([
+ [{ id: "dp-97" }, "Unnamed Site (dp-97)"],
+ [{ id: "dp-97", name: "Deskpro Site" }, "Deskpro Site"],
+ [
+ { id: "dp-97", name: "Deskpro Site", brandingName: "Deskpro Site (Branding)" },
+ "Deskpro Site (Branding)",
+ ],
+ ])("correctly handles site name fallbacks", (site, target) => {
+ const { getByText } = renderComponent(site)
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add more edge cases for site name/branding fallbacks (e.g. empty strings and undefined values).
The current cases cover only the basic fallbacks. Please add a few rows for more realistic inputs, such as `brandingName: ""` with a non-empty `name`, and scenarios where `name` and/or `brandingName` are empty strings/whitespace or `null`/`undefined`, so the fallback behavior is clearly defined and guarded by tests.
```suggestion
describe("Filters", () => {
it.each([
// No name or brandingName -> fallback to "Unnamed Site (id)"
[{ id: "dp-97" }, "Unnamed Site (dp-97)"],
// Name only -> use name
[{ id: "dp-97", name: "Deskpro Site" }, "Deskpro Site"],
// Branding name only -> use brandingName
[{ id: "dp-97", brandingName: "Deskpro Site (Branding)" }, "Deskpro Site (Branding)"],
// Both name and brandingName non-empty -> prefer brandingName
[
{ id: "dp-97", name: "Deskpro Site", brandingName: "Deskpro Site (Branding)" },
"Deskpro Site (Branding)",
],
// Empty brandingName, non-empty name -> fall back to name
[
{ id: "dp-97", name: "Deskpro Site", brandingName: "" },
"Deskpro Site",
],
// Whitespace brandingName, non-empty name -> fall back to name
[
{ id: "dp-97", name: "Deskpro Site", brandingName: " " },
"Deskpro Site",
],
// Empty name, non-empty brandingName -> use brandingName
[
{ id: "dp-97", name: "", brandingName: "Deskpro Site (Branding)" },
"Deskpro Site (Branding)",
],
// Whitespace name, non-empty brandingName -> use brandingName
[
{ id: "dp-97", name: " ", brandingName: "Deskpro Site (Branding)" },
"Deskpro Site (Branding)",
],
// Empty name and brandingName -> fall back to "Unnamed Site (id)"
[
{ id: "dp-97", name: "", brandingName: "" },
"Unnamed Site (dp-97)",
],
// Null brandingName, non-empty name -> fall back to name
[
{
id: "dp-97",
name: "Deskpro Site",
brandingName: null as unknown as string,
},
"Deskpro Site",
],
// Null name and brandingName -> fall back to "Unnamed Site (id)"
[
{
id: "dp-97",
name: null as unknown as string,
brandingName: null as unknown as string,
},
"Unnamed Site (dp-97)",
],
])("correctly handles site name fallbacks", (site, target) => {
const { getByText } = renderComponent(site)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+23
to
+32
| describe("Filters", () => { | ||
| it.each([ | ||
| [{ id: "dp-97" }, "Unnamed Site (dp-97)"], | ||
| [{ id: "dp-97", name: "Deskpro Site" }, "Deskpro Site"], | ||
| [ | ||
| { id: "dp-97", name: "Deskpro Site", brandingName: "Deskpro Site (Branding)" }, | ||
| "Deskpro Site (Branding)", | ||
| ], | ||
| ])("correctly handles site name fallbacks", (site, target) => { | ||
| const { getByText } = renderComponent(site) |
There was a problem hiding this comment.
suggestion (testing): Add more edge cases for site name/branding fallbacks (e.g. empty strings and undefined values).
The current cases cover only the basic fallbacks. Please add a few rows for more realistic inputs, such as brandingName: "" with a non-empty name, and scenarios where name and/or brandingName are empty strings/whitespace or null/undefined, so the fallback behavior is clearly defined and guarded by tests.
Suggested change
| describe("Filters", () => { | |
| it.each([ | |
| [{ id: "dp-97" }, "Unnamed Site (dp-97)"], | |
| [{ id: "dp-97", name: "Deskpro Site" }, "Deskpro Site"], | |
| [ | |
| { id: "dp-97", name: "Deskpro Site", brandingName: "Deskpro Site (Branding)" }, | |
| "Deskpro Site (Branding)", | |
| ], | |
| ])("correctly handles site name fallbacks", (site, target) => { | |
| const { getByText } = renderComponent(site) | |
| describe("Filters", () => { | |
| it.each([ | |
| // No name or brandingName -> fallback to "Unnamed Site (id)" | |
| [{ id: "dp-97" }, "Unnamed Site (dp-97)"], | |
| // Name only -> use name | |
| [{ id: "dp-97", name: "Deskpro Site" }, "Deskpro Site"], | |
| // Branding name only -> use brandingName | |
| [{ id: "dp-97", brandingName: "Deskpro Site (Branding)" }, "Deskpro Site (Branding)"], | |
| // Both name and brandingName non-empty -> prefer brandingName | |
| [ | |
| { id: "dp-97", name: "Deskpro Site", brandingName: "Deskpro Site (Branding)" }, | |
| "Deskpro Site (Branding)", | |
| ], | |
| // Empty brandingName, non-empty name -> fall back to name | |
| [ | |
| { id: "dp-97", name: "Deskpro Site", brandingName: "" }, | |
| "Deskpro Site", | |
| ], | |
| // Whitespace brandingName, non-empty name -> fall back to name | |
| [ | |
| { id: "dp-97", name: "Deskpro Site", brandingName: " " }, | |
| "Deskpro Site", | |
| ], | |
| // Empty name, non-empty brandingName -> use brandingName | |
| [ | |
| { id: "dp-97", name: "", brandingName: "Deskpro Site (Branding)" }, | |
| "Deskpro Site (Branding)", | |
| ], | |
| // Whitespace name, non-empty brandingName -> use brandingName | |
| [ | |
| { id: "dp-97", name: " ", brandingName: "Deskpro Site (Branding)" }, | |
| "Deskpro Site (Branding)", | |
| ], | |
| // Empty name and brandingName -> fall back to "Unnamed Site (id)" | |
| [ | |
| { id: "dp-97", name: "", brandingName: "" }, | |
| "Unnamed Site (dp-97)", | |
| ], | |
| // Null brandingName, non-empty name -> fall back to name | |
| [ | |
| { | |
| id: "dp-97", | |
| name: "Deskpro Site", | |
| brandingName: null as unknown as string, | |
| }, | |
| "Deskpro Site", | |
| ], | |
| // Null name and brandingName -> fall back to "Unnamed Site (id)" | |
| [ | |
| { | |
| id: "dp-97", | |
| name: null as unknown as string, | |
| brandingName: null as unknown as string, | |
| }, | |
| "Unnamed Site (dp-97)", | |
| ], | |
| ])("correctly handles site name fallbacks", (site, target) => { | |
| const { getByText } = renderComponent(site) |
|
Build for commit e3de0d6 deployed to: https://lansweeper-pr-66.ci.next.deskprodemo.com URLs: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR replaces site ID with site name in Filters component and add tests for site name fallbacks.
Evidence
Summary by Sourcery
Update the LinkDevice Filters component to use human-friendly site name fallbacks and add coverage for the new behavior.
Bug Fixes:
Tests: