docs: update subscription flow, add OAuth renewal priority, brainstor… #62
Conversation
WalkthroughThis PR migrates persistence from SQLite to PostgreSQL/Drizzle across the GitHub integration docs and schemas. SQL definitions for github_installations, installation_repositories, github_subscriptions, and webhook_deliveries are updated (timestamps to TIMESTAMPTZ, subscription PK renamed, booleans, new FKs and cascade semantics, added fields like enabled, event_types, retry_count). Documentation (GITHUB_APP_IMPLEMENTATION_PLAN.md, README.md) is reworked to describe an OAuth-first flow, pending-subscription lifecycle, GitHub App installation requirements for private repos, and token storage via github_user_tokens. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
GITHUB_APP_IMPLEMENTATION_PLAN.md (1)
440-440: Capitalize "GitHub" in two locations.Lines 440 and 451 refer to "github" but the official name is "GitHub" with capital H. Flagged by LanguageTool.
Apply this diff:
- Line ~440: `mirrors `/github subscribe`` → should be `/github subscribe` (this is a command, not the platform name, so this may be a false positive) - Line ~451: `**Installation status command**: `/github app-status`` → same as above (command reference) However, if the references are to the platform itself (not commands), apply: ```diff - "so the UX mirrors /github subscribe" + "so the UX mirrors `/github subscribe`"Note: This may be a false positive by LanguageTool since both occurrences appear to be slash command references (e.g., `/github subscribe`) rather than references to the GitHub platform itself. Verify intent before applying. Also applies to: 451-451 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 8ca76dc890c6436506e663208ee309c75c9b4140 and b2a50257aa85c6aa872d80d92757fc700878b46f. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `GITHUB_APP_IMPLEMENTATION_PLAN.md` (10 hunks) * `README.md` (5 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (2)</summary> <details> <summary>📚 Learning: 2025-11-17T23:50:17.552Z</summary>Learnt from: shuhuiluo
Repo: HereNotThere/bot-github PR: 26
File: src/github-app/app.ts:1-3
Timestamp: 2025-11-17T23:50:17.552Z
Learning: The HereNotThere/bot-github project uses Bun as its package manager, with dependencies locked in bun.lock rather than package-lock.json.**Applied to files:** - `README.md` - `GITHUB_APP_IMPLEMENTATION_PLAN.md` </details> <details> <summary>📚 Learning: 2025-11-18T23:35:49.436Z</summary>Learnt from: shuhuiluo
Repo: HereNotThere/bot-github PR: 26
File: src/db/index.ts:61-69
Timestamp: 2025-11-18T23:35:49.436Z
Learning: Inwebhook_deliveriestable (src/db/index.ts), theinstallation_idcolumn should NOT have a FOREIGN KEY constraint because the table serves as an immutable audit log for idempotency tracking. Records must persist independently even after installations are deleted, and a foreign key would create race conditions when webhooks arrive before installation records are created. The field is intentionally nullable to support webhooks without installation context.**Applied to files:** - `GITHUB_APP_IMPLEMENTATION_PLAN.md` </details> </details><details> <summary>🪛 LanguageTool</summary> <details> <summary>GITHUB_APP_IMPLEMENTATION_PLAN.md</summary> [uncategorized] ~440-~440: The official name of this software platform is spelled with a capital “H”. Context: ...e dedicated responses so the UX mirrors `/github subscribe`. ### References - `src/han... (GITHUB) --- [uncategorized] ~451-~451: The official name of this software platform is spelled with a capital “H”. Context: ...amp 2. **Installation status command**: `/github app-status` to view all installations 3... (GITHUB) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>GITHUB_APP_IMPLEMENTATION_PLAN.md</summary> 223-223: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 247-247: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 348-348: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 463-463: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 495-495: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>🔇 Additional comments (13)</summary><blockquote> <details> <summary>README.md (5)</summary><blockquote> `32-33`: **Excellent documentation of OAuth-first architecture and private repo support.** The features clearly distinguish real-time webhooks from polling and properly emphasize the GitHub App requirement for private repositories. The content aligns well with the implementation plan. --- `150-150`: **Clear OAuth flow description.** The explanation accurately captures the subscription creation during callback completion and success page handoff. This properly reflects the single OAuth flow mentioned in the PR objectives. --- `159-159`: **Private repository requirement properly documented.** The distinction that private repos require GitHub App installation while public repos can fall back to polling is clearly stated and consistent with the implementation plan. --- `220-226`: **Completed items section aligns with PR objectives.** Pending subscriptions and improved subscription UX are correctly listed as completed. However, the PR objectives mention removing "admin detection" from completed features (not used), but this file does not show that item being removed—please verify if it should appear here or elsewhere. --- `228-230`: **Priority section with OAuth token renewal properly added.** This accurately reflects the PR objective to add OAuth token renewal as a priority remaining work item, and the section structure is clear. </blockquote></details> <details> <summary>GITHUB_APP_IMPLEMENTATION_PLAN.md (8)</summary><blockquote> `70-71`: **Database-backed state architecture documented.** The design decision around database persistence and CASCADE cleanup is clearly stated. This properly sets context for the schema changes that follow and aligns with the PostgreSQL migration mentioned in the PR summary. --- `108-129`: **Schema updates for `github_subscriptions` are comprehensive.** The changes correctly document: - Primary key rename to `id SERIAL PRIMARY KEY` - `is_private BOOLEAN` type change - New foreign key to `github_user_tokens(towns_user_id)` with CASCADE - `installation_id` with SET NULL (allowing subscriptions to persist after app uninstall) - `enabled` flag and `event_types` defaults - TIMESTAMPTZ for created/updated timestamps - Unique constraint on (space_id, channel_id, repo_full_name) The schema aligns with the README's OAuth and private repository support documentation. --- `149-149`: **Audit trail design for `webhook_deliveries` is correct.** The note that `installation_id` is nullable with no foreign key constraint properly documents the immutable audit log pattern. This preserves webhook records even after installations are deleted, preventing race conditions when webhooks arrive before installation records—aligning with the architectural principle. Based on learnings, this design choice is intentionally correct and should not be changed. --- `217-241`: **Public Repository Flow is clearly documented.** The step-by-step flow from subscription command through OAuth, GitHub App detection, and polling fallback is easy to follow. The auto-redirect to GitHub App installation (line 238) and optional automatic upgrade (line 240) are clearly stated. This properly implements the single OAuth flow objective. --- `243-271`: **Private Repository Flow properly separates public vs. private concerns.** The requirement that private repos must have GitHub App installed is clearly enforced (line 245). The pending subscriptions lifecycle (lines 257–270) with the 1-hour expiration, auto-completion on installation webhook, and cleanup mechanism is well-documented and matches the PR objective to document pending subscriptions. --- `287-296`: **Installation Webhook Handler section documents upgrade path.** The description of how installation events trigger subscription upgrades from polling to webhook mode and completion of pending subscriptions is clear and actionable for implementation. This supports the private repository flow documented above. --- `348-357`: **OAuth Token Renewal documented as Priority with implementation details.** This properly addresses the PR objective to add OAuth token renewal as a priority remaining work item. The implementation guidance (check expiration, use `oauth.refreshToken()`, update database, fall back to reauth) is actionable. Line 352 correctly notes that the schema already has `refreshToken` and `refreshTokenExpiresAt` columns, which supports the approach. --- `457-507`: **`/gh_stat` command brainstorm is comprehensive.** This properly fulfills the PR objective to add the `/gh_stat` command brainstorm including: - **Subcommands** (lines 461–468): Overview, contributors, activity, leaderboard - **API sources** (lines 470–477): Five GitHub REST endpoints documented with specific data returned - **Leaderboard ideas** (lines 479–485): Five gamified metrics with emoji - **Caching considerations** (lines 487–491): GitHub 202 retry logic and 1-hour cache strategy - **Display format mockup** (lines 493–507): Realistic example with GitHub repo showing formatted output The brainstorm is well-structured and immediately actionable for future implementation. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
…m /gh_stat - Separate public vs private repo subscription flows with pending subscriptions - Remove 'admin detection' from completed features (not used) - Add OAuth token renewal as priority remaining work - Add /gh_stat command brainstorm with API sources and display mockup - Fix markdown lint issues (MD040 code fence language, MD036 emphasis as heading)
shuhuiluo
force-pushed
the
docs/update
branch
from
b2a5025 to
4459c17
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
GITHUB_APP_IMPLEMENTATION_PLAN.md (4)
217-219: Clarify production status language.Lines 217–219 state "Single OAuth Flow (Shipped)" and "Live in production" but the context is a plan document. The phrasing "users only leave Towns once and the subscription is created automatically during the OAuth callback" is ambiguous—does this mean users authorize once and tokens are reused, or tokens are refreshed?
Consider rephrasing for clarity:
- ### Single OAuth Flow (Shipped) + ### OAuth Flow (Current Implementation) - **Status:** Live in production — users only leave Towns once and the subscription is created automatically during the OAuth callback. + **Status:** Deployed in production. Users authorize once; the subscription is created automatically during OAuth callback. Tokens are refreshed automatically (see Priority: OAuth Token Renewal).This clarifies that OAuth is one-time, subscriptions are auto-created, and token refresh is a known improvement.
487-491: Remove or definerepo_stats_cachetable reference.Lines 487–491 mention "consider storing in
repo_stats_cachetable" as a caching strategy for/gh_statstatistics. However, this table is not defined in the database schema section (lines 74–150), making this reference premature for an aspirational feature.Either:
- Remove the specific table reference (preferred for aspirational work):
- Consider storing in `repo_stats_cache` table + Consider storing results in a cache table (e.g., repo_stats_cache)Or, if committing to the schema now:
2. Add a schema definition forrepo_stats_cachein the Database Schema section.Since
/gh_statis marked as future work, keeping schema definition out of this doc is cleaner.
428-430: Improve phrasing of section heading.Line 428–430: "Query Commands Private Repo Support" is an awkward noun phrase. Consider a more natural heading.
- ## Query Commands Private Repo Support + ## Private Repository Support for Query CommandsOr alternatively:
- ## Query Commands Private Repo Support + ## Query Commands: Private Repository AccessThis improves readability without changing meaning.
114-115: Inline schema documentation can be enhanced for clarity, but external reference is sufficient.The
github_user_tokenstable is properly defined insrc/db/schema.tswith all expected fields:accessToken(encrypted),tokenType,expiresAt,refreshToken, andrefreshTokenExpiresAt. Line 130 already references the external schema definition, and line 352 explicitly mentions the refresh token fields.For documentation completeness, consider adding an inline schema definition block for
github_user_tokens(similar togithub_installationsshown at lines 77–86) to make the plan self-contained. This would list all columns explicitly and improve readability, but is optional since the external reference insrc/db/schema.tsis already documented and accurate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
GITHUB_APP_IMPLEMENTATION_PLAN.md(10 hunks)README.md(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-18T23:35:49.436Z
Learnt from: shuhuiluo
Repo: HereNotThere/bot-github PR: 26
File: src/db/index.ts:61-69
Timestamp: 2025-11-18T23:35:49.436Z
Learning: In `webhook_deliveries` table (src/db/index.ts), the `installation_id` column should NOT have a FOREIGN KEY constraint because the table serves as an immutable audit log for idempotency tracking. Records must persist independently even after installations are deleted, and a foreign key would create race conditions when webhooks arrive before installation records are created. The field is intentionally nullable to support webhooks without installation context.
Applied to files:
GITHUB_APP_IMPLEMENTATION_PLAN.md
📚 Learning: 2025-11-17T23:50:17.552Z
Learnt from: shuhuiluo
Repo: HereNotThere/bot-github PR: 26
File: src/github-app/app.ts:1-3
Timestamp: 2025-11-17T23:50:17.552Z
Learning: The HereNotThere/bot-github project uses Bun as its package manager, with dependencies locked in bun.lock rather than package-lock.json.
Applied to files:
GITHUB_APP_IMPLEMENTATION_PLAN.md
🪛 LanguageTool
GITHUB_APP_IMPLEMENTATION_PLAN.md
[uncategorized] ~440-~440: The official name of this software platform is spelled with a capital “H”.
Context: ...e dedicated responses so the UX mirrors /github subscribe. ### References - `src/han...
(GITHUB)
[uncategorized] ~451-~451: The official name of this software platform is spelled with a capital “H”.
Context: ...amp 2. Installation status command: /github app-status to view all installations 3...
(GITHUB)
🔇 Additional comments (3)
GITHUB_APP_IMPLEMENTATION_PLAN.md (3)
350-357: Verify schema columns for OAuth token refresh.Line 352 assumes
refreshTokenandrefreshTokenExpiresAtcolumns exist in the schema ("schema already has columns"), but these columns are not shown in any of the provided SQL definitions (github_subscriptions, github_user_tokens).Confirm whether:
- These columns already exist in
github_user_tokensschema (which is defined insrc/db/schema.ts), or- They need to be added to the schema and documented here.
If they exist, update this section to explicitly reference which table stores them.
108-108: The schema shown in the plan is the final, implemented state—not a pending breaking change.The GITHUB_APP_IMPLEMENTATION_PLAN.md document (lines 108–123) describes the final
github_subscriptionstable structure, which is already:
- Implemented in
src/db/schema.tsvia Drizzle ORM- Deployed via migration files in
./drizzle/(specifically0000_postgres_init.sql)- Used throughout the codebase with no references to
subscription_idThere are no breaking changes to document. The table uses
id SERIAL PRIMARY KEY(not a rename fromsubscription_id), and migration scripts already exist. The document correctly describes the finalized schema design.Likely an incorrect or invalid review comment.
428-446: Additional verification needed. Let me check the two remaining claims:Now let me check the implementation details of the files referenced:
Let me do a direct inspection of the implementation files:
Verified: Private repo query command implementation aligns with documentation.
Script execution confirmed all referenced components:
- ✓ Files exist:
src/handlers/gh-pr-handler.ts,src/handlers/gh-issue-handler.ts,src/utils/oauth-helpers.ts,src/api/github-client.ts- ✓
classifyApiError()defined and used in both handlers for error classification- ✓
forbiddenandnot_founderror handling present in both handlers (lines 66, 72, 148, 156)- ✓
getUserOctokit()called as OAuth fallback in both handlers- ✓ Implementation matches documentation: bot token first → error classification → user token retry → error response
The documented OAuth fallback strategy for
/gh_prand/gh_issuecommands is correctly implemented.
…m /gh_stat
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.