Skip to content

Fix "Fill PR description" workflow failure when OPENAI_API_KEY is missing - #3

Open
orelkrylatiy wants to merge 4 commits into
mainfrom
KAN-6
Open

Fix "Fill PR description" workflow failure when OPENAI_API_KEY is missing#3
orelkrylatiy wants to merge 4 commits into
mainfrom
KAN-6

Conversation

@orelkrylatiy

@orelkrylatiy orelkrylatiy commented Jun 8, 2026

Copy link
Copy Markdown
Owner

What was done

  • Analyzed GitHub Actions job logs for check run 80551979332 and identified the failing point in .github/scripts/update-pr-description.mjs.
  • Updated .github/workflows/ai-pr-description.yml to run the “Generate and update PR description” step only when secrets.OPENAI_API_KEY is set:
    • if: ${{ secrets.OPENAI_API_KEY != '' }}

Why

  • The workflow was failing because OPENAI_API_KEY was not configured, and the script throws an error when it is missing.
  • This change prevents unnecessary CI failures while preserving existing behavior when the secret is available.

How to check

  1. Run the AI PR Description workflow in an environment without OPENAI_API_KEY and confirm the generation step is skipped (job does not fail on missing secret).
  2. Run the workflow with OPENAI_API_KEY configured and confirm the generation step executes normally.
  3. Verify the rest of the workflow remains unchanged and successful.

Screenshots / Demo

Not applicable.

Checklist

  • I checked that the app runs locally
  • I reviewed my own diff before requesting review
  • I updated docs/specs if behavior changed
  • I placed files according to the project structure
  • I avoided unnecessary abstractions
  • I did not leave unused code, logs or commented code
  • I added/updated tests if needed

Code Review Principles

Code review is a collaboration to improve the project, not a competition or personal criticism.

For reviewer

  • Focus on code, not the author.
  • Comment only when the comment helps the project.
  • Explain why the change is important.
  • Prefer questions and suggestions over commands.
  • Praise good solutions.
  • Mark blocking and non-blocking comments clearly.

For author

  • Treat review as learning.
  • Ask questions if something is unclear.
  • Reply to comments after changes.
  • Explain what was changed.
  • Do self-review before requesting review.

Review comment tags

Main tags

Tag Meaning Blocking? Example
no tag Blocking issue. Requires fix or answer. Yes "This can break rendering when students is empty."
q Question. Can be resolved by explanation. Resolver: reviewer. Yes "q: Why is this state stored instead of derived?"
s Suggestion. Author decides whether to apply. No "s: This helper can be extracted later."

Additional tags

Tag Meaning Blocking? Example
praise Positive feedback. No "praise: Nice separation between entity and page."
note Context or useful information. No "note: We use CSS Modules for component styles."
nit Small style/comment issue. No "nit: This name can be shorter."
thought Idea for future iterations. No "thought: Later this can become a feature slice."

@orelkrylatiy
orelkrylatiy force-pushed the KAN-6 branch 3 times, most recently from d194a16 to 927d42a Compare June 8, 2026 18:23
Comment thread vite.config.ts
Comment thread src/pages/StatisticPage.tsx
Comment thread src/widgets/NavBar.tsx
Copilot AI changed the title KAN-6 Fix "Fill PR description" workflow failure when OPENAI_API_KEY is missing Jun 14, 2026
className?: string;
}

export const ActionCard = ({ title, description }: Props) => {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🚫 Blocker — className declared but never applied

className is in the Props interface but the destructuring on this line only picks title and description, so passing className has no effect on the rendered output.

Either forward it:

export const ActionCard = ({ title, description, className }: Props) => {
  return <div className={cx(styles.card, className)}>

or remove it from the interface.

export const ActionCard = ({ title, description }: Props) => {
return (
<div className={styles.card}>
<Logo />

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🚫 Blocker — <Logo> called without required src prop

Logo's src: string is required (not optional), so <Logo /> is a TypeScript error and the rendered <img> has no source. Every ActionCard will show a broken image.

Pass a meaningful image path, or make src optional in Logo with a fallback:

// Option A: pass actual src
<Logo src={iconSrc} />

// Option B: make src optional in Logo
src?: string;
// then: src={src || ''}

getPullRequestDiff(),
readOptionalFile("docs/review/code-review-guide.md"),
readOptionalFile(".github/copilot-instructions.md"),
readOptionalFile('docs/review/code-review-guide.md'),

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🚫 Blocker — Wrong path for code-review guide

docs/review/code-review-guide.md does not exist in this repo. readOptionalFile silently returns an empty string on a missing path (the catch block swallows the error), so every generated PR description is built with "No review guide found." as context.

The file was added to .ai/code-review-guide.md in this same PR. Fix:

readOptionalFile('.ai/code-review-guide.md'),

Comment thread tsconfig.tsbuildinfo
@@ -0,0 +1 @@
{"fileNames":[],"fileInfos":[],"root":[],"version":"6.0.3"} No newline at end of file

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

💡 Suggestion — Remove compiled artifact from git

tsconfig.tsbuildinfo is a TypeScript incremental build cache generated locally by tsc --build. It should not be committed — it contains machine-specific paths and will produce a spurious diff on every developer machine.

Add to .gitignore:

*.tsbuildinfo

Then remove the file from tracking:

git rm --cached tsconfig.tsbuildinfo

Comment thread .husky/pre-commit
@@ -1,2 +1 @@
npm run test
npx lint-staged No newline at end of file

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

💡 Suggestion — Test step removed from pre-commit hook

The hook now only runs lint-staged, dropping the test gate. AGENTS.md (added in this same PR) explicitly documents npm run test as a required pre-commit step and lists it in the Git workflow section.

The hook and the docs are inconsistent from the moment this is merged. If removing tests was intentional (e.g., to speed up commits), either update AGENTS.md to reflect this, or scope Vitest to only changed files via lint-staged:

"*.{ts,tsx}": ["vitest related --run"]

Comment thread vite.config.ts
environment: 'jsdom',
globals: true,
setupFiles: "",
setupFiles: '',

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

💡 Suggestion — setupFiles: '' leaves test setup unwired

An empty string means Vitest loads nothing. src/shared/config/tests/setupTests.ts (which imports @testing-library/jest-dom) is never executed, so matchers like toBeInTheDocument() will throw at runtime.

Fix:

setupFiles: ['src/shared/config/tests/setupTests.ts'],

Or remove setupFiles entirely and add the import to a dedicated setup entry instead.

@orelkrylatiy

Copy link
Copy Markdown
Owner Author

Review Summary

The PR description says it's a one-liner workflow fix (if: ${{ secrets.OPENAI_API_KEY != '' }}), but the actual diff is 56 files — the full initial app scaffold, CI scripts, docs, and agent skills. The workflow fix itself is correct; the concerns below are about the accompanying changes.

Blockers (must fix before merge):

  • src/shared/ui/ActionCard.tsx:11className prop declared but never applied (silent styling breakage for callers)
  • src/shared/ui/ActionCard.tsx:14<Logo /> missing required src prop (TypeScript error + broken image on every ActionCard)
  • .github/scripts/update-pr-description.mjs:176 — reads review guide from docs/review/code-review-guide.md which doesn't exist; actual file is .ai/code-review-guide.md

Suggestions:

  • tsconfig.tsbuildinfo — committed build artifact; add *.tsbuildinfo to .gitignore and untrack it
  • .husky/pre-commit — test gate removed but AGENTS.md (in this same PR) still documents it as required; reconcile the two
  • vite.config.ts:10setupFiles: '' leaves setupTests.ts unwired; Vitest never loads @testing-library/jest-dom

Note on PR scope: The checklist item "I reviewed my own diff before requesting review" is unchecked, and the PR description only describes the workflow change — a reviewer approaching this PR cold has no stated intent for the other 55 files. Worth a self-review pass and description update before merge.

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.

2 participants