Conversation
47b984b to
7b8deb7
Compare
|
/strands review |
Code Review SummaryI've reviewed PR #950 and found several issues that need to be addressed before merge. Overall, the architecture is clean and the config-driven approach is excellent, but there are three blocking issues that must be fixed. ⛔ Blocking Issues1. Missing
|
|
/strands review |
Code Review SummaryI've reviewed PR #950 and I'm pleased to report that the previous blocking issues have been fully addressed. The code is well-structured, follows good engineering practices, and includes comprehensive tests. I have a few suggestions for improvement, but none are blocking. ✅ Previous Issues Resolved
💡 Suggestions (Non-Blocking)1. CI Workflow OptimizationFile: The workflow fetches ~900 CI runs, which may hit GitHub API rate limits under certain conditions. Consider: - name: Generate dashboard
working-directory: .github/dashboard
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: |
# Add rate limit check before generation
REMAINING=$(gh api rate_limit --jq '.resources.core.remaining')
if [ "$REMAINING" -lt 100 ]; then
echo "Warning: Only $REMAINING API calls remaining"
fi
npx tsx src/generate.tsAlso, the 2. Error Handling for Missing Workflow NamesFile: Good validation was added for empty workflow matches! One additional edge case to consider: what if the workflows array in config is empty? export function fetchCIRuns(repo: string, workflowNames: string[], branch: string, maxRuns: number): WorkflowRun[] {
if (workflowNames.length === 0) {
throw new Error('No workflow names provided in CI configuration');
}
// ... rest of implementation
}3. Test Coverage for Edge CasesFile: The tests are excellent! A few additional cases to consider:
Example: it('handles empty buckets array', () => {
const result = buildHistogram([5, 10], []);
expect(result).toEqual([]);
});4. Client-Side TypeScript DisabledFile: /* eslint-disable */
// @ts-nocheckThis is understandable since placeholders are injected at build time, but it means 466 lines have zero type safety. A safer approach would be to:
Not blocking, but worth considering for future maintenance. 5. Dependency Version PinningFile: The previous review suggested pinning exact versions. You've kept caret ranges: "devDependencies": {
"@types/node": "20.17.57", // No caret - good!
"tsx": "4.19.4", // No caret - good!
"typescript": "5.8.3", // No caret - good!
"vitest": "3.2.1" // No caret - good!
}Actually, I see you did pin exact versions — well done! The package-lock.json will ensure reproducible builds. 6. Documentation EnhancementFile: The README covers local development well. Consider adding:
The previous review had a good architecture section example that would fit nicely here. 🎯 What I Liked
📋 SummaryThis is ready to merge after addressing the previous review feedback. All three blocking issues are resolved:
The suggestions above are minor improvements for maintainability and robustness, not blockers. Recommendation: ✅ Approve (pending final CI checks) Great work on the dashboard! The visualizations in the screenshots look polished, and the code quality is high. This will be a valuable tool for tracking repo health. |
…e handlers, use esbuild for TS strip, tighten types
Problem
The team has no centralized view of issue health, PR review velocity, or CI reliability for the agentcore-cli repo.
GitHub's built-in Insights are shallow: no time-to-review histograms, no label gap analysis, no CI flaky job detection.
Solution
Adds a dashboard generator under
.github/dashboard/that produces three HTML pages (Issues, PRs, CI/Tests) deployed to GitHub Pages via a new workflow.Architecture:
src/config.ts) — add sections/pages by editing config, no code changes neededghCLIThe page will automatically update on each issue, PR, and once a day.
Testing
Screenshots:

