Skip to content

docs: add low-effort PR examples and quality expectations document (C… #203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Code-Paragon merged 1 commit into from
Aug 13, 2026
Merged

docs: add low-effort PR examples and quality expectations document (C… #203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ Key resources for contributors:

- [Evaluation Readiness Dashboard](docs/evaluation-readiness.md) — Central summary page for testing standards, CI workflow, PR evidence, acceptance criteria mapping, self-review, and payment guidance
- [Contributing Guide](CONTRIBUTING.md) — Branch naming, component rules, PR evidence checklist, and review process
- [Low-Effort PR Examples](docs/low-effort-pr-examples.md) — Examples of superficial, under-tested, or unsafe PRs that will fail evaluation
- [Evaluation-Readiness Index](docs/evaluation-readiness-index.md) — Central page linking every requirement for GrantFox evaluation: payment expectations, testing standards, CI guidance, acceptance criteria audit, self-assessment, and reviewer checklist
- [Issue Approval Readiness Checklist](docs/issue-approval-readiness-checklist.md) — Pre-evaluation checklist for contributors and reviewers to verify implementation completeness, testing, CI, acceptance criteria, documentation, and known limitations before GrantFox evaluation
- [PR Evidence Checklist](docs/pr-evidence-checklist.md) — Detailed requirements for pull request evidence and documentation
Expand Down
284 changes: 51 additions & 233 deletions docs/low-effort-pr-examples.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,262 +1,80 @@
# Aegis Dashboard — Low-Effort PR Examples
# Low-Effort PR Examples & Quality Expectations

This document shows common PR submissions that are unlikely to pass GrantFox evaluation and how to improve them. Each section pairs a minimal, checklist-only submission with an improved version that demonstrates real effort.
This document outlines common examples of "low-effort" pull requests in the aegis-dashboard repository. PRs exhibiting these anti-patterns will typically fail maintainer evaluation and be closed.

---

## 1. Low-Effort UI Changes Without Screenshots

UI-only changes are among the easiest to under-document. A PR that touches `.tsx` or `.css` files but lacks visual proof is immediately flagged.

### Minimal submission

> **PR Title:** "Fix button alignment on portfolio page"
>
> **Description:**
> - [x] Fixed button alignment
> - [x] Tested locally
>
> **Files changed:** `src/components/AssetCard.tsx`
>
> **Screenshots:** _none_

**Why it fails:**
- No before/after screenshots — reviewer cannot verify the visual change
- Single-line description gives zero context on what was wrong or how it was fixed
- No indication of which browsers or viewports were tested
- Checklist items are generic and unverifiable

### Improved version

> **PR Title:** "Fix asset card button misalignment on mobile viewports"
>
> **Description:**
> - Asset card transfer buttons overflowed their container at widths below 400px
> - Replaced `gap-4` with `gap-2` and added `flex-wrap` to the card actions row
> - Verified on Chrome, Firefox, and Safari at 375px, 414px, and 390px
> - [x] Buttons remain clickable and do not overlap card borders
> - [x] Desktop layout unchanged (regression check at 1280px+)
>
> **Files changed:** `src/components/AssetCard.tsx`
>
> **Screenshots:**
> | Before (375px) | After (375px) |
> |---|---|
> | ![before](https://example.com/before.png) | ![after](https://example.com/after.png) |

**What makes it better:**
- Before/after screenshots at the breakpoint that broke
- Specific description of the root cause and the exact CSS change
- Browser and viewport test matrix stated explicitly
- Regression check confirmation for larger viewports
- Checklist items are concrete and verifiable
To ensure your contributions are valuable, thoroughly reviewed, and merged, study the contrast between unacceptable submissions and their expected alternatives.

---

## 2. Under-Tested PRs

PRs that add logic, state changes, or API calls without corresponding test coverage or manual verification evidence.

### Minimal submission

> **PR Title:** "Add transfer validation for large amounts"
>
> **Description:**
> - Added max-amount check to the transfer modal
> - It works when I tested it
>
> **Files changed:** `src/components/TransferModal.tsx`
>
> **Tests added:** _none_
>
> **Manual testing:** "Tried a big number, got an error"
## 1. Superficial or Cosmetic Changes

**Why it fails:**
- No unit or integration tests for new validation logic
- Manual testing description is vague — which values were tried? What error appeared?
- No edge case documentation (zero, negative, empty, NaN)
- Single file change with no corresponding test file
**The Anti-Pattern:** Submitting a PR that solely fixes a minor typo in a comment, reorders imports without a structural reason, or tweaks markdown formatting, while claiming a feature or substantial bounty.

### Improved version
**Unacceptable Example:**
```tsx
// Changes:
// - // Check if the wallet is connected
// + // Checks if the wallet is connected
```

> **PR Title:** "Enforce transfer amount ceiling of 10,000 tokens"
>
> **Description:**
> - Transfers exceeding 10,000 tokens now show a validation error before the transaction is submitted
> - Validation runs client-side in `validateTransfer()` before the Freighter prompt
> - Added 6 test cases covering the full boundary
>
> **Files changed:**
> - `src/components/TransferModal.tsx`
> - `src/components/__tests__/TransferModal.test.tsx`
>
> **Tests added:**
> ```ts
> describe('transfer amount ceiling', () => {
> it('allows 10,000 tokens (at ceiling)', () => { ... });
> it('rejects 10,001 tokens (above ceiling)', () => { ... });
> it('allows 0 tokens (zero amount)', () => { ... });
> it('rejects negative amounts', () => { ... });
> it('rejects non-numeric input', () => { ... });
> it('allows 1 token (normal amount)', () => { ... });
> });
> ```
>
> **Manual testing:**
> | Input | Expected result | Observed |
> |---|---|---|
> | 10,000 | Transfer proceeds | ✅ |
> | 10,001 | "Amount exceeds maximum" error | ✅ |
> | 0 | "Amount must be greater than 0" | ✅ |
> | -5 | "Amount must be greater than 0" | ✅ |
> | "abc" | Input prevented (type="number") | ✅ |

**What makes it better:**
- Test file added with 6 explicit boundary cases
- Manual test table with input → expected → observed columns
- Validation logic name and location stated clearly
- Edge cases covered: ceiling boundary, zero, negative, non-numeric
**Expected Alternative:**
If you notice typos, batch them together across the entire codebase or include them as a secondary commit in a PR that delivers meaningful, tested UI changes.

---

## 3. Screenshot-Free UI Examples

This section shows realistic, concrete examples of what screenshot-free UI PRs look like — and what the improved version should include.

### Example A: Layout shift fix
## 2. Under-Tested Logic (Missing Coverage)

**Minimal submission:**
> Fixed a layout shift on the admin page. Tested, looks correct.
**The Anti-Pattern:** Adding or modifying core components, custom hooks, or state transitions in the React frontend without providing corresponding unit tests or visual regression fixtures.

**Improved version:**
> Fixed a layout shift on the admin page caused by the whitelist input loading spinner pushing the mint-asset card downward. Wrapped both action cards in a `min-h-[320px]` container with `items-start`. Verified no shift during loading state on Chrome 126 (375px, 768px, 1440px).
>
> | Before (loading state) | After (loading state) |
> |---|---|
> | ![before](...) | ![after](...) |
**Unacceptable Example:**
```tsx
// Adding a new compliance check hook without testing it
export function useComplianceCheck(address: string) {
// Implementation added...
// But no .test.ts file created!
}
```

### Example B: Color contrast update

**Minimal submission:**
> Updated error text color for WCAG compliance.

**Improved version:**
> Updated transfer error message color from `text-red-400` (#f87171, contrast 3.2:1) to `text-red-600` (#dc2626, contrast 5.8:1) to meet WCAG AA 4.5:1 minimum. Light mode only — dark mode already passes.
>
> | Element | Before | After |
> |---|---|---|
> | Error text (light) | ![before](...) | ![after](...) |
> | Error text (dark) | ![unchanged](...) | ![unchanged](...) |

### Example C: New component

**Minimal submission:**
> Added a badge component for KYC status. Works on the portfolio page.

**Improved version:**
> Added `KycBadge` component showing "Verified" / "Pending" / "Rejected" states. Uses green/amber/red color coding. Integrated into `AssetCard` header.
>
> | State | Screenshot |
> |---|---|
> | Verified | ![verified](...) |
> | Pending | ![pending](...) |
> | Rejected | ![rejected](...) |
>
> Responsive check: badge scales correctly at 375px, 768px, 1440px.

### Example D: Dark mode toggle

**Minimal submission:**
> Added dark mode support. All pages look fine.

**Improved version:**
> Added dark mode toggle via `next-themes`. Swapped hardcoded Tailwind colors for `dark:` variants across 4 components. Toggle button placed in navbar.
>
> | Page | Light | Dark |
> |---|---|---|
> | Home | ![home-light](...) | ![home-dark](...) |
> | Portfolio | ![portfolio-light](...) | ![portfolio-dark](...) |
> | Admin | ![admin-light](...) | ![admin-dark](...) |
**Expected Alternative:**
Every new hook or major component must include robust Vitest tests covering both happy and failure paths.

---

## 4. Failing-CI Examples

PRs submitted while CI is red — either ignored, unnoticed, or dismissed with a weak justification.

### Minimal submission

> **PR Title:** "Refactor asset fetching hook"
>
> **CI status:** ❌ 2 checks failing
>
> **Author comment:** "Tests pass locally, CI is flaky"
>
> **Failing checks:**
> - `lint` — 4 errors (unused imports, missing dependency in useEffect)
> - `typecheck` — 1 error (wrong prop type in AssetCard)

**Why it fails:**
- "CI is flaky" is not a valid justification when lint and type errors are deterministic
- Author did not investigate or fix the failures before opening the PR
- Forces reviewers to triage CI failures instead of reviewing the actual change

### Improved version

> **PR Title:** "Refactor asset fetching hook"
>
> **CI status:** ✅ All checks passing
>
> **Fixes applied before opening PR:**
> - Removed unused `useCallback` import flagged by lint
> - Added `fetchAssets` to `useEffect` dependency array
> - Updated `AssetCard` props interface to accept `isLoading?: boolean`
>
> **Verification:**
> ```
> npm run lint → 0 errors, 0 warnings
> npm run typecheck → 0 errors
> npm run test → 14 passed, 0 failed
> ```
## 3. Unsafe State Mutations or Suppressed Errors

**What makes it better:**
- All CI checks pass before requesting review
- Specific fixes listed for each failure that was resolved
- Terminal output pasted as proof of clean runs
- No reviewer time wasted on avoidable CI issues
**The Anti-Pattern:** Using `@ts-ignore`, `any`, or suppressing ESLint rules to bypass type safety, or swallowing errors silently in catch blocks.

### Common failing-CI patterns and how to fix them
**Unacceptable Example:**
```tsx
catch (err) {
// @ts-ignore
console.log(err); // Swallows the error silently, breaking the UI state
}
```

| Failure type | Before submitting | After fixing |
|---|---|---|
| **Lint errors** | "Will fix later" | Run `npm run lint -- --fix`, commit result |
| **Type errors** | "Types are wrong / too strict" | Fix types or narrow with proper guards, never use `as any` |
| **Test failures** | "Tests need updating" | Update tests or explain why the old test is no longer valid |
| **Build failures** | "Builds locally" | Rebase on `main`, clean install, re-run build |
| **E2E flakes** | "CI is unreliable" | Re-run once; if it fails again, investigate and fix root cause |
**Expected Alternative:**
```tsx
catch (err) {
const wrappedErr = err instanceof Error ? err : new Error(String(err));
setError(wrappedErr.message); // Properly surfaces the error to the user
}
```

---

## Submission Checklist
## 4. Failing CI / Ignored Checks

Before opening a PR, confirm:
**The Anti-Pattern:** Opening a PR, noticing the GitHub Actions CI pipeline fails, and requesting a maintainer review anyway.

- [ ] **UI changes** include before/after screenshots (light + dark mode if applicable)
- [ ] **Logic changes** include unit tests or a manual test table with input/expected/observed
- [ ] **New components** show screenshots of every visual state (loading, empty, error, populated)
- [ ] **CI is green** — `lint`, `typecheck`, and `test` all pass
- [ ] **PR description** explains _what_ changed, _why_, and _how_ it was verified
- [ ] **No single-line descriptions** — even small fixes need context
**Expected Alternative:**
Before requesting review, ensure your local build and tests pass. If CI fails after pushing, inspect the logs, fix the formatting, linter warnings, or broken tests, and push the corrections.

---

## Quick Reference
## Summary of a "High-Effort" PR

| Red flag | Why it's a problem |
|---|---|
| No screenshots on a UI PR | Reviewer cannot verify the visual change |
| "Tested locally" with no details | Unverifiable — no evidence of what was tested |
| CI is red at PR open | Wastes reviewer time; suggests author didn't check their own work |
| One-line PR description | Leaves reviewer guessing about intent and scope |
| No test file for logic changes | No automated regression protection |
| "Works on my machine" | Ignores environment differences; CI is the source of truth |
1. **Meaningful:** Solves a documented issue.
2. **Tested:** Includes robust positive and negative Vitest tests.
3. **Safe:** Uses strict TypeScript typing.
4. **Green CI:** Passes all local and remote checks.
5. **Descriptive:** Explains what changed and why.
Loading