Skip to content

Conversation

@QingengWei
Copy link
Contributor

@QingengWei QingengWei commented Dec 11, 2025

When result is bool type, it will throw:

return result.get("data") if "data" in result else result

TypeError: argument of type 'bool' is not iterable

Greptile Overview

Greptile Summary

Fixed a type safety issue in http_interceptor where checking for the "data" key in API responses could fail or behave incorrectly when the response is a list, string, or integer instead of a dictionary.

Key changes:

  • Added isinstance(result, dict) check before accessing the "data" key in HTTP response handling
  • Prevents unexpected behavior when API returns non-dict JSON types (lists, strings, integers)
  • Addresses a real bug since some endpoints return lists directly (e.g., task list endpoint returns list[SimulationTask])

Missing:

  • No changelog entry added (required by project standards for non-refactor PRs)

Confidence Score: 4/5

  • This PR is safe to merge with minor considerations
  • The code change is correct and fixes a real type safety bug. The logic is sound and improves robustness. Score reduced by 1 point due to missing changelog entry which is required by project standards.
  • No files require special attention - the change is straightforward and correct

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/web/core/http_util.py 5/5 Added type guard to prevent checking for dict keys on non-dict types (lists, strings, integers)

Sequence Diagram

sequenceDiagram
    participant Client as HTTP Client
    participant Wrapper as http_interceptor wrapper
    participant API as External API
    participant Logger as Logger

    Client->>Wrapper: HTTP request (get/post/put/delete)
    Wrapper->>API: Forward request
    API-->>Wrapper: HTTP Response
    
    alt Response is not 200 OK
        alt Response is 404 and suppress_404=True
            Wrapper-->>Client: Return None
        else Response is 404 and suppress_404=False
            Wrapper-->>Client: Raise WebNotFoundError
        else Other error status
            Wrapper-->>Client: Raise WebError with details
        end
    else Response is 200 OK
        alt Response body is empty
            Wrapper-->>Client: Return None
        else Response has body
            Wrapper->>Wrapper: Parse JSON response
            alt Result is dict with "warning" key
                Wrapper->>Logger: Log warning
            end
            alt Result is dict with "data" key [FIXED]
                Wrapper-->>Client: Return result["data"]
            else Result is list/str/int or dict without "data" [FIXED]
                Wrapper-->>Client: Return result as-is
            end
        end
    end
Loading

Context used:

  • Rule from dashboard - Require a changelog entry for any PR that is not purely an internal refactor. (source)

Copilot AI review requested due to automatic review settings December 11, 2025 09:04
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR optimizes the result handling logic in the HTTP utility wrapper by replacing a conditional expression with a more explicit if-else structure that type-checks the result before accessing dictionary keys.

Key Changes:

  • Refactored the result return statement to use explicit if-else blocks with type checking

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

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@QingengWei QingengWei changed the title result optimization SCRF-2132: result optimization Dec 11, 2025
@github-actions
Copy link
Contributor

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

  • tidy3d/web/core/http_util.py (100%)

Summary

  • Total: 3 lines
  • Missing: 0 lines
  • Coverage: 100%

@momchil-flex momchil-flex added this pull request to the merge queue Dec 11, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 11, 2025
@momchil-flex
Copy link
Collaborator

Thanks @QingengWei . We've started enforcing some rules, your PR title correctly starts with a ticket number now, but there's also the rule that commit messages should follow conventional commits, which fails. Could you just make your commit start with fix:. Thanks.

@QingengWei QingengWei changed the title SCRF-2132: result optimization fix: SCRF-2132: result optimization Dec 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants