Refactor api/view.catch_all to reduce cyclomatic complexity by ma-maiorova · Pull Request #123 · kittinan/spotify-github-profile

ma-maiorova · 2025-12-05T22:14:49Z

Extracted query parameter parsing into a dedicated ViewParams dataclass (api/view_params.py).
Moved image- and color-related helpers into view_utils.py (e.g. loading cover image, base64 encoding, bar color extraction, artist/song resolution).
Simplified catch_all into a linear sequence of small steps (validation, offline handling, redirect, cover loading, bar color selection, SVG rendering).
Kept the public API and function names intact so existing tests and integrations continue to work.

Copilot

Pull request overview

This PR successfully refactors the catch_all function in api/view.py to reduce cyclomatic complexity by extracting helper functions and introducing structured parameter handling. The refactoring maintains backward compatibility while improving code organization and readability.

Key Changes:

Introduced ViewParams dataclass to encapsulate all query parameters in a structured format
Extracted image processing, color extraction, and artist/song resolution logic into dedicated utility functions in view_utils.py
Simplified the main catch_all flow into a clear, linear sequence of operations

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.

File	Description
api/view_params.py	New dataclass to hold parsed view query parameters, providing type safety and cleaner parameter passing
api/view_utils.py	New utility module containing extracted helper functions for image loading, base64 encoding, cover image handling, bar color extraction, and artist/song name resolution
api/view.py	Refactored main view handler with simplified logic flow, new `parse_view_params()` and `build_offline_response()` functions, and removal of inline logic in favor of utility function calls

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-05T22:18:41Z

api/view_utils.py

+        return ""
+    return b64encode(content).decode("ascii")
+
+def load_cover_image_if_needed(cover_image, currently_playing_type, item, load_image_func, to_img_b64_func,):


Function signature has inconsistent trailing comma formatting. Line 34 has a trailing comma after to_img_b64_func, but line 49 has a trailing comma after colorgram_module,. This is inconsistent and can be confusing. Consider either removing all trailing commas from function parameter lists or applying them consistently only to the last parameter.

Copilot · 2025-12-05T22:18:42Z

api/view.py

-        song_name = item["name"]
+    bar_color = params.bar_color
+    if params.is_bar_color_from_cover and img is not None:
+        bar_color = extract_bar_color_from_image(img, params.theme, bar_color, isLightOrDark, colorgram)


The function extract_bar_color_from_image is called with isLightOrDark and colorgram as parameters, but these are passed as module/function references rather than being called within the utility function itself. This creates an unusual dependency injection pattern that makes the code harder to understand and test. Consider importing and using isLightOrDark directly within extract_bar_color_from_image in view_utils.py, similar to how other dependencies are handled.

Suggested change

bar_color = extract_bar_color_from_image(img, params.theme, bar_color, isLightOrDark, colorgram)

bar_color = extract_bar_color_from_image(img, params.theme, bar_color)

Copilot · 2025-12-05T22:18:42Z

api/view_utils.py

+@functools.lru_cache(maxsize=128)
+def load_image(url):
+    try:
+        response = requests.get(url, timeout=10)
+        response.raise_for_status()
+        return response.content
+    except requests.exceptions.RequestException as e:
+        print(f"Error loading image from {url}: {e}")
+        # Return a placeholder or None to handle gracefully
+        return None
+    except Exception as e:
+        print(f"Unexpected error loading image: {e}")
+        return None
+
+def to_img_b64(content):
+    if content is None:
+        return ""
+    return b64encode(content).decode("ascii")
+
+def load_cover_image_if_needed(cover_image, currently_playing_type, item, load_image_func, to_img_b64_func,):
+    if not cover_image:
+        return None, ""
+
+    img = None
+    if currently_playing_type == "track":
+        img = load_image_func(item["album"]["images"][1]["url"])
+    elif currently_playing_type == "episode":
+        img = load_image_func(item["images"][1]["url"])
+
+    if img is None:
+        return None, ""
+
+    return img, to_img_b64_func(img)
+
+def extract_bar_color_from_image(img, theme, default_bar_color, isLightOrDark_func, colorgram_module,):
+    is_skip_dark = theme in ["default"]
+
+    try:
+        pil_img = Image.open(io.BytesIO(img))
+        colors = colorgram_module.extract(pil_img, 5)
+    except Exception as e:
+        print(f"Error extracting colors from image: {e}")
+        return default_bar_color
+
+    for color in colors:
+        rgb = color.rgb
+        light_or_dark = isLightOrDark_func([rgb.r, rgb.g, rgb.b], threshold=80)
+
+        if light_or_dark == "dark" and is_skip_dark:
+            # Skip to use bar in dark color
+            continue
+
+        return "%02x%02x%02x" % (rgb.r, rgb.g, rgb.b)
+
+    return default_bar_color
+
+def resolve_artist_and_song_names(item, currently_playing_type):
+    if currently_playing_type == "track":
+        return item["artists"][0]["name"], item["name"]
+    if currently_playing_type == "episode":
+        return item["show"]["publisher"], item["name"]
+    return "", ""


The newly introduced utility functions in api/view_utils.py (load_image, to_img_b64, load_cover_image_if_needed, extract_bar_color_from_image, resolve_artist_and_song_names) and the dataclass ViewParams lack test coverage. Since the repository has comprehensive test coverage for the api/view.py module (as seen in tests/test_api_view.py), these newly extracted functions should also have dedicated unit tests to ensure their behavior is properly validated, especially edge cases like missing image indices or None returns.

Copilot · 2025-12-05T22:18:42Z

api/view_utils.py

+def load_cover_image_if_needed(cover_image, currently_playing_type, item, load_image_func, to_img_b64_func,):
+    if not cover_image:
+        return None, ""
+
+    img = None
+    if currently_playing_type == "track":
+        img = load_image_func(item["album"]["images"][1]["url"])
+    elif currently_playing_type == "episode":
+        img = load_image_func(item["images"][1]["url"])
+
+    if img is None:
+        return None, ""
+
+    return img, to_img_b64_func(img)


[nitpick] Parameter names load_image_func and to_img_b64_func use _func suffix which is redundant and verbose. Since these are dependency injection parameters, consider more concise names like image_loader and image_encoder, or simply remove the suffix: load_image and to_img_b64 (which wouldn't shadow the actual functions in this context).

Suggested change

def load_cover_image_if_needed(cover_image, currently_playing_type, item, load_image_func, to_img_b64_func,):

if not cover_image:

return None, ""

img = None

if currently_playing_type == "track":

img = load_image_func(item["album"]["images"][1]["url"])

elif currently_playing_type == "episode":

img = load_image_func(item["images"][1]["url"])

if img is None:

return None, ""

return img, to_img_b64_func(img)

def load_cover_image_if_needed(cover_image, currently_playing_type, item, load_image, to_img_b64,):

if not cover_image:

return None, ""

img = None

if currently_playing_type == "track":

img = load_image(item["album"]["images"][1]["url"])

elif currently_playing_type == "episode":

img = load_image(item["images"][1]["url"])

if img is None:

return None, ""

return img, to_img_b64(img)

Copilot · 2025-12-05T22:18:42Z

api/view.py

+def parse_view_params():
+    uid = request.args.get("uid")
+    return ViewParams(
+        uid=uid,
+        cover_image=request.args.get("cover_image", default="true") == "true",
+        is_redirect=request.args.get("redirect", default="false") == "true",
+        theme=request.args.get("theme", default="default"),
+        bar_color=request.args.get("bar_color", default="53b14f"),
+        background_color=request.args.get("background_color", default="121212"),
+        is_bar_color_from_cover=(
+                request.args.get("bar_color_cover", default="false") == "true"
+        ),
+        show_offline=request.args.get("show_offline", default="false") == "true",
+        interchange=request.args.get("interchange", default="false") == "true",
+        mode=request.args.get("mode", default="light"),
+        is_enable_profanity=request.args.get("profanity", default="false") == "true",
+    )


The parse_view_params function lacks test coverage. Since the repository has comprehensive test coverage for view.py (as seen in tests/test_api_view.py), this newly extracted parsing logic should have dedicated unit tests to validate parameter parsing, especially edge cases like missing parameters, invalid boolean strings, and default values.

Copilot · 2025-12-05T22:18:43Z

api/view.py

+def build_offline_response(params, is_now_playing, progress_ms, duration_ms):
+    if params.interchange:
+        artist_name = "Currently not playing on Spotify"
+        song_name = "Offline"
+    else:
+        artist_name = "Offline"
+        song_name = "Currently not playing on Spotify"
+
+    svg = make_svg(
+        artist_name,
+        song_name,
+        img_b64="",
+        is_now_playing=is_now_playing,
+        cover_image=False,
+        theme=params.theme,
+        bar_color=params.bar_color,
+        show_offline=params.show_offline,
+        background_color=params.background_color,
+        mode=params.mode,
+        progress_ms=progress_ms,
+        duration_ms=duration_ms,
+    )
+    resp = Response(svg, mimetype="image/svg+xml")
+    resp.headers["Cache-Control"] = "s-maxage=1"
+    return resp


The build_offline_response function lacks test coverage. While the offline behavior is tested in the existing tests (e.g., test_catch_all_with_valid_uid_offline), the new extracted function itself should have dedicated unit tests to ensure its logic is properly isolated and tested, including the interchange parameter handling.

Copilot · 2025-12-05T22:18:43Z

api/view_utils.py

+        img = load_image_func(item["album"]["images"][1]["url"])
+    elif currently_playing_type == "episode":
+        img = load_image_func(item["images"][1]["url"])


The load_cover_image_if_needed function assumes that images have at least 2 items (index [1]) without validation. If the Spotify API returns an album or episode with fewer than 2 images, this will raise an IndexError. Consider adding a check: if len(item["album"]["images"]) >= 2: before accessing [1].

Suggested change

img = load_image_func(item["album"]["images"][1]["url"])

elif currently_playing_type == "episode":

img = load_image_func(item["images"][1]["url"])

images = item["album"]["images"]

if len(images) >= 2:

img = load_image_func(images[1]["url"])

elif len(images) >= 1:

img = load_image_func(images[0]["url"])

elif currently_playing_type == "episode":

images = item["images"]

if len(images) >= 2:

img = load_image_func(images[1]["url"])

elif len(images) >= 1:

img = load_image_func(images[0]["url"])

Copilot · 2025-12-05T22:18:43Z

api/view.py

-            bar_color = "%02x%02x%02x" % (rgb.r, rgb.g, rgb.b)
-            break
+    img, img_b64 = load_cover_image_if_needed(
+        params.cover_image, currently_playing_type, item, load_image, to_img_b64,


Trailing comma after to_img_b64, on line 367. While trailing commas in function calls can be acceptable, this appears inconsistent with the rest of the codebase. Consider removing it for consistency.

Suggested change

params.cover_image, currently_playing_type, item, load_image, to_img_b64,

params.cover_image, currently_playing_type, item, load_image, to_img_b64

Copilot · 2025-12-05T22:18:43Z

api/view_utils.py

+def extract_bar_color_from_image(img, theme, default_bar_color, isLightOrDark_func, colorgram_module,):
+    is_skip_dark = theme in ["default"]
+
+    try:
+        pil_img = Image.open(io.BytesIO(img))
+        colors = colorgram_module.extract(pil_img, 5)
+    except Exception as e:
+        print(f"Error extracting colors from image: {e}")
+        return default_bar_color
+
+    for color in colors:
+        rgb = color.rgb
+        light_or_dark = isLightOrDark_func([rgb.r, rgb.g, rgb.b], threshold=80)


Parameter names isLightOrDark_func and colorgram_module use inconsistent naming conventions. isLightOrDark_func uses camelCase with _func suffix, while colorgram_module uses snake_case with _module suffix. For consistency and Python conventions (PEP 8), use snake_case: is_light_or_dark_func or better yet, light_dark_checker and color_extractor for more descriptive names.

Suggested change

def extract_bar_color_from_image(img, theme, default_bar_color, isLightOrDark_func, colorgram_module,):

is_skip_dark = theme in ["default"]

try:

pil_img = Image.open(io.BytesIO(img))

colors = colorgram_module.extract(pil_img, 5)

except Exception as e:

print(f"Error extracting colors from image: {e}")

return default_bar_color

for color in colors:

rgb = color.rgb

light_or_dark = isLightOrDark_func([rgb.r, rgb.g, rgb.b], threshold=80)

def extract_bar_color_from_image(img, theme, default_bar_color, light_dark_checker, color_extractor,):

is_skip_dark = theme in ["default"]

try:

pil_img = Image.open(io.BytesIO(img))

colors = color_extractor.extract(pil_img, 5)

except Exception as e:

print(f"Error extracting colors from image: {e}")

return default_bar_color

for color in colors:

rgb = color.rgb

light_or_dark = light_dark_checker([rgb.r, rgb.g, rgb.b], threshold=80)

Copilot · 2025-12-05T22:18:44Z

api/view.py

+        img_b64="",
+        is_now_playing=is_now_playing,
+        cover_image=False,
+        theme=params.theme,
+        bar_color=params.bar_color,
+        show_offline=params.show_offline,
+        background_color=params.background_color,
+        mode=params.mode,
+        progress_ms=progress_ms,
+        duration_ms=duration_ms,


Keyword argument 'img_b64' is not a supported parameter name of function make_svg.

Suggested change

img_b64="",

is_now_playing=is_now_playing,

cover_image=False,

theme=params.theme,

bar_color=params.bar_color,

show_offline=params.show_offline,

background_color=params.background_color,

mode=params.mode,

progress_ms=progress_ms,

duration_ms=duration_ms,

"",

is_now_playing,

False,

params.theme,

params.bar_color,

params.show_offline,

params.background_color,

params.mode,

progress_ms,

duration_ms,

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

api/view.py

api/view_utils.py

refactor func catch_all

92fe04c

Copilot AI review requested due to automatic review settings December 5, 2025 22:14

Copilot started reviewing on behalf of ma-maiorova December 5, 2025 22:15 View session

Copilot AI reviewed Dec 5, 2025

View reviewed changes

ma-maiorova requested a review from Copilot December 5, 2025 22:21

Copilot started reviewing on behalf of ma-maiorova December 5, 2025 22:21 View session

Copilot AI reviewed Dec 5, 2025

View reviewed changes

api/view.py Show resolved Hide resolved

api/view_utils.py Show resolved Hide resolved

api/view_utils.py Show resolved Hide resolved

api/view_utils.py Show resolved Hide resolved

	bar_color = extract_bar_color_from_image(img, params.theme, bar_color, isLightOrDark, colorgram)
	bar_color = extract_bar_color_from_image(img, params.theme, bar_color)

-        img = load_image_func(item["album"]["images"][1]["url"])
-    elif currently_playing_type == "episode":
-        img = load_image_func(item["images"][1]["url"])
+        images = item["album"]["images"]
+        if len(images) >= 2:
+            img = load_image_func(images[1]["url"])
+        elif len(images) >= 1:
+            img = load_image_func(images[0]["url"])
+    elif currently_playing_type == "episode":
+        images = item["images"]
+        if len(images) >= 2:
+            img = load_image_func(images[1]["url"])
+        elif len(images) >= 1:
+            img = load_image_func(images[0]["url"])

	params.cover_image, currently_playing_type, item, load_image, to_img_b64,
	params.cover_image, currently_playing_type, item, load_image, to_img_b64

Conversation

ma-maiorova commented Dec 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant