Skip to content

Conversation

hiroshinishio
Copy link
Contributor

No description provided.

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @hiroshinishio - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider adding a null check or default state for selectedRepo initialization since githubRepos[0] could be undefined if no repositories are found
  • Add error state handling in the UI to show feedback to users when repository fetching fails, rather than just logging to console
Here's what I looked at during the review
  • 🟡 General issues: 2 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 1 issue found
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

// Get repository list where GitAuto is installed
const repositories = ["gitautoai/gitauto", "gitautoai/gitauto-jira"];
const [selectedRepo, setSelectedRepo] = useState(repositories[0]);
const [selectedRepo, setSelectedRepo] = useState(githubRepos[0]);
Copy link

Choose a reason for hiding this comment

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

suggestion: Consider handling the case where githubRepos is empty to avoid setting undefined as the initial value

You could set it to null or provide a placeholder value when the array is empty.

Suggested change
const [selectedRepo, setSelectedRepo] = useState(githubRepos[0]);
const [selectedRepo, setSelectedRepo] = useState(githubRepos.length > 0 ? githubRepos[0] : null);

);
} catch (error) {
console.error("Error fetching repositories:", error);
setGithubRepos([]);
Copy link

Choose a reason for hiding this comment

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

suggestion: Add user-facing error feedback when repository fetching fails

Consider showing an error message in the UI to inform users when repositories cannot be loaded.

          setGithubRepos([]);
          setError("Unable to load repositories. Please try again later.");

import { invoke } from "@forge/bridge";

const App = () => {
// Get Jira cloud ID (== workspace ID)
Copy link

Choose a reason for hiding this comment

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

issue (complexity): Consider consolidating the multiple state variables and effects into a single custom hook for managing GitHub repository data

The current implementation uses multiple chained useEffect hooks and state variables that could be simplified into a single custom hook. Here's how to reduce complexity while maintaining functionality:

const useGithubRepos = () => {
  const context = useProductContext();
  const [githubRepos, setGithubRepos] = useState([]);
  const [selectedRepo, setSelectedRepo] = useState(null);

  useEffect(() => {
    const fetchRepositories = async () => {
      if (!context) return;

      try {
        const response = await invoke("getGithubRepos", {
          cloudId: context.cloudId,
          projectId: context.extension.project.id,
        });

        const repos = response.map(
          (repo) => `${repo.github_owner_name}/${repo.github_repo_name}`
        );
        setGithubRepos(repos);
        setSelectedRepo(repos[0]); // Set initial selection
      } catch (error) {
        console.error("Error fetching repositories:", error);
        setGithubRepos([]);
      }
    };

    fetchRepositories();
  }, [context]);

  return { githubRepos, selectedRepo, setSelectedRepo };
};

Then simplify the App component:

const App = () => {
  const { githubRepos, selectedRepo, setSelectedRepo } = useGithubRepos();

  return (
    <>
      <Text>Target GitHub Repository:</Text>
      <Select
        value={selectedRepo}
        onChange={setSelectedRepo}
        options={githubRepos.map((repo) => ({ label: repo, value: repo }))}
      />
    </>
  );
};

This approach:

  • Eliminates intermediate state variables
  • Reduces the number of useEffect hooks
  • Encapsulates related logic in a reusable hook
  • Maintains the same functionality with less complexity

@hiroshinishio hiroshinishio merged commit 82ad165 into main Dec 7, 2024
1 check passed
@hiroshinishio hiroshinishio deleted the wes branch December 7, 2024 11:37
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.

1 participant