feat!: add support for GH discussions #71
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes introduce comprehensive support for GitHub Discussions across the application. This includes updating type definitions, server-side caching, and retrieval logic, as well as extending UI components and rendering logic to handle discussions alongside issues and pull requests. Parameter handling and route logic are also updated to recognize and process discussions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TrackerPage as Tracker Page (+page.svelte)
participant ServerLoader as Server Loader (+page.server.ts)
participant GitHubCache
participant GitHubAPI
User->>TrackerPage: Load tracker page
TrackerPage->>ServerLoader: load({ params })
ServerLoader->>GitHubCache: getAllPullRequests()
ServerLoader->>GitHubCache: getAllIssues()
ServerLoader->>GitHubCache: getAllDiscussions()
GitHubCache->>GitHubAPI: Fetch PRs, Issues, Discussions
GitHubAPI-->>GitHubCache: Return data
GitHubCache-->>ServerLoader: Return PRs, Issues, Discussions
ServerLoader-->>TrackerPage: Return filtered and sorted PRs, Issues, Discussions
TrackerPage->>User: Render lists with badges for PRs, Issues, Discussions
sequenceDiagram
participant User
participant ItemPage as Item Page ([pid=pid]/.../+page.svelte)
participant ItemServer as Item Loader ([pid=pid]/.../+page.server.ts)
participant GitHubCache
User->>ItemPage: Open item (PR/Issue/Discussion) page
ItemPage->>ItemServer: load({ params })
ItemServer->>GitHubCache: getItemDetails(type, id)
GitHubCache->>GitHubCache: Try PR, Issue, Discussion in order
GitHubCache->>GitHubAPI: Fetch item details and comments
GitHubAPI-->>GitHubCache: Return item data
GitHubCache-->>ItemServer: Return item details
ItemServer-->>ItemPage: Return item data
ItemPage->>User: Render item with badge, metadata, threaded comments (if discussion)
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/routes/[pid=pid]/[org]/[repo]/[id=number]/PageRenderer.svelteOops! Something went wrong! :( ESLint: 9.26.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@sveltejs/adapter-vercel' imported from /svelte.config.js src/routes/tracker/[org]/[repo]/+page.svelteOops! Something went wrong! :( ESLint: 9.26.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@sveltejs/adapter-vercel' imported from /svelte.config.js src/lib/server/github-cache.tsOops! Something went wrong! :( ESLint: 9.26.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@sveltejs/adapter-vercel' imported from /svelte.config.js Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
WarningImHack3r
force-pushed
the
discussions-support
branch
from
4324792 to
151ec67
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (11)
src/params/pid.ts (1)
1-3: Support for GitHub Discussions verifiedThe function correctly validates the three supported entity types: "pull", "issues", and "discussions". This addition aligns with the PR objective of integrating GitHub Discussions.
Consider for future extensibility:
-export function match(param: string) { - return param === "pull" || param === "issues" || param === "discussions"; -} +export function match(param: string) { + const validTypes = ["pull", "issues", "discussions"]; + return validTypes.includes(param); +}src/routes/[pid=pid]/[org]/[repo]/[id=number]/+page.server.ts (1)
23-28: Type mapping expanded for discussionsThe type mapping now properly handles three cases, converting "discussions" to "discussion" similar to how "issues" becomes "issue".
For improved readability, consider using an object map:
- type: - type === "issues" - ? ("issue" as const) - : type === "discussions" - ? ("discussion" as const) - : ("pull" as const) + type: { + issues: "issue", + discussions: "discussion", + pull: "pull" + }[type] as constsrc/routes/tracker/[org]/[repo]/+page.server.ts (1)
51-56: Discussion filtering implementationThe discussions are correctly filtered to include only those authored by members and updated within the last year.
Note that there's an inconsistency in filtering criteria between entity types - discussions are filtered by recency (last year), but PRs and issues aren't. If this was intentional, consider adding a comment explaining the rationale.
src/lib/components/GHBadge.svelte (2)
11-14: Avoid shadowing the importedIcontype for clearer type-checkingYou import
type Iconpurely for typing, yet later assign a local reactive var withtypeof Icon. Although this works, the identical names can confuse editors and readers. Consider aliasing the import to make its purpose obvious, e.g.import { type Icon as LucideIcon } …and then usetypeof LucideIconwhen declaringicon.
95-111: Verify colour semantics and future-proof status mapping for “discussion” badgesThe newly added “discussion” branch uses purple for “closed”, which differs from the grey used for closed issues and red for closed PRs. If the intent was to mirror GitHub’s grey treatment for closed discussions, switch to the neutral palette; if purple was intentional, add a comment to clarify.
In addition, if GitHub later introduces “answered”/“answered-and-locked” states, consider mapping them now so callers don’t need to change their
statusstrings later.src/routes/tracker/[org]/[repo]/+page.svelte (2)
77-82:categoryproperty check could mis-classify on future GitHub payload changes
"category" in itemis currently unique to discussions, but GitHub may add acategoryfield to other resources (e.g. enhanced issues).
Defensive pattern-matching on thehtml_url(/discussions/), or on a sentinel such asrepository_url.endsWith('/discussions'), would make the type discrimination more robust.
170-175: Safer link construction & null-guarding for empty discussion arrays
- If the server ever returns
undefinedinstead of[],data.discussions.lengthwill throw. Prefer the optional-chaining formdata.discussions?.length.- You derive
ownerSlashRepofromrepository_url; the REST payload also provideshtml_url, which already contains the correct owner/repo path and is less error-prone (no regex orreplaceneeded).-const ownerSlashRepo = d.repository_url.replace("https://api.github.com/repos/", ""); -return `/discussions/${ownerSlashRepo}/${d.number}`; +return new URL(d.html_url).pathname.replace(/^\/|\/discussion.*$/g, "");src/routes/[pid=pid]/[org]/[repo]/[id=number]/PageRenderer.svelte (2)
548-555: Update CTA label for discussionsThe call-to-action still reads “issue” for discussions, which may confuse users.
-Open {metadata.type === "pull" ? "pull request" : "issue"} on GitHub +Open {metadata.type === "pull" + ? "pull request" + : metadata.type === "discussion" + ? "discussion" + : "issue"} on GitHub
365-372: Indent calculation can be simplified & made explicit
isAnsweractually checks for any nested reply, not specifically an accepted answer.
A clearer flag name avoids future confusion and reduces the ternary complexity.-{@const isAnswer = - "parent_id" in comment && comment.parent_id ? comment.parent_id !== info.id : false} +{@const isNestedReply = "parent_id" in comment && comment.parent_id !== null}Then use
isNestedReplyfor border/indent styling.src/lib/server/github-cache.ts (2)
55-59: Missinglinked*fields inDiscussionDetailsFor symmetry with
IssueDetailsandPullRequestDetails, consider addinglinkedIssues/linkedPrs(or at least a placeholder) toDiscussionDetails. Consumers that already assume these keys exist for the other item types will otherwise need special-case branching.
832-856:getAllDiscussionssilently swallows API failuresCatching all errors and returning an empty array makes it hard to distinguish “no discussions” from “GitHub is down / token revoked”.
At minimum, log the error so operators can trace the problem:- } catch { - return [] as Discussion[]; + } catch (err) { + console.error(`Error fetching discussions for ${owner}/${repo}:`, err); + return [] as Discussion[]; }Optionally propagate the error and let callers decide whether to fall back.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/lib/components/GHBadge.svelte(3 hunks)src/lib/server/github-cache.ts(7 hunks)src/params/pid.ts(1 hunks)src/params/poi.ts(0 hunks)src/routes/[pid=pid]/[org]/[repo]/[id=number]/+page.server.ts(1 hunks)src/routes/[pid=pid]/[org]/[repo]/[id=number]/+page.svelte(1 hunks)src/routes/[pid=pid]/[org]/[repo]/[id=number]/PageRenderer.svelte(6 hunks)src/routes/tracker/[org]/[repo]/+page.server.ts(2 hunks)src/routes/tracker/[org]/[repo]/+page.svelte(6 hunks)
💤 Files with no reviewable changes (1)
- src/params/poi.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: WarningImHack3r
PR: WarningImHack3r/svelte-changelog#56
File: src/routes/package/+layout.server.ts:28-40
Timestamp: 2025-04-25T10:58:24.062Z
Learning: WarningImHack3r prefers elegant, aesthetically pleasing solutions for performance optimizations rather than quick fixes that introduce nested async IIFEs or similar approaches that might make code harder to read.
Learnt from: WarningImHack3r
PR: WarningImHack3r/svelte-changelog#48
File: src/routes/package/[...package]/+page.server.ts:50-53
Timestamp: 2025-04-11T10:33:07.002Z
Learning: WarningImHack3r prefers to keep server-side console.log statements in the codebase as they find them useful for debugging and monitoring, even if they might clean them up later.
🔇 Additional comments (7)
src/routes/[pid=pid]/[org]/[repo]/[id=number]/+page.svelte (1)
9-14: More robust linked entities handlingThe improved check prevents potential undefined access errors when neither
linkedPrsnorlinkedIssuesexists indata.item. This is good defensive programming.src/routes/[pid=pid]/[org]/[repo]/[id=number]/+page.server.ts (3)
5-5: Improved parameter namingChanging from
pullOrIssuetopidreflects the expanded support for discussions alongside pulls and issues.
13-13: Added type detection for discussionsThe new type detection logic correctly identifies discussions by checking for the
categoryproperty initem.info.
15-15: Changed redirect status codeYou've changed the redirect status from 303 to 307. This preserves the HTTP method during redirection, which is particularly important for POST requests.
src/routes/tracker/[org]/[repo]/+page.server.ts (2)
23-26: Added discussions fetchingThe code now properly fetches discussions alongside PRs and issues, using Promise.all for concurrent execution.
47-49: Refined issue filteringThe issue filtering logic is now more explicit, excluding pull requests and including only those with author_association "MEMBER".
src/lib/server/github-cache.ts (1)
26-35: Use the same singular/plural convention everywhereYou added both
"discussion"and"discussions"toRepoKeyType, which is great.
However, across the rest of the file:
- Lists are cached with the plural key (
"issues","prs"), and single-item details are cached with the singular key ("issue","pr").getDiscussionDetails()stores details under the singular key ("discussion"), butgetItemDetails()expects the plural discriminator ("discussions").This asymmetry is easy to miss and will break callers that pass
"discussion"(they will hit the fallback logic and do three API calls instead of one).- type: ExtractStrict<RepoKeyType, "issue" | "pr" | "discussions"> | undefined = undefined + type: ExtractStrict<RepoKeyType, "issue" | "pr" | "discussion"> | undefined = undefinedand in the switch:
- case "discussions": + case "discussion":Please grep the codebase for every call to
getItemDetailsto ensure call sites use the updated literal.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/routes/tracker/[org]/[repo]/+page.svelte (1)
167-172: Consider standardizing URL construction approachThe implementation for rendering discussions works well, but uses a different URL construction approach compared to issues (which use html_url). This inconsistency might lead to maintenance challenges.
Consider standardizing the URL construction approach across all item types for better maintainability:
{#if data.discussions.length} {@render list("Discussions", data.discussions, d => { - const ownerSlashRepo = d.repository_url.replace("https://api.github.com/repos/", ""); - return `/discussions/${ownerSlashRepo}/${d.number}`; + const ownerSlashRepo = d.html_url + ? d.html_url.replace("https://github.com/", "").replace(/\/[A-z]+\/\d+$/, "") + : d.repository_url.replace("https://api.github.com/repos/", ""); + return `/discussions/${ownerSlashRepo}/${d.number}`; })} {/if}This assumes that discussions have an
html_urlproperty like issues do. If they don't, then the current approach is acceptable as a fallback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.json(3 hunks)src/lib/server/github-cache.ts(7 hunks)src/routes/[pid=pid]/[org]/[repo]/[id=number]/PageRenderer.svelte(6 hunks)src/routes/tracker/[org]/[repo]/+page.svelte(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/[pid=pid]/[org]/[repo]/[id=number]/PageRenderer.svelte
- src/lib/server/github-cache.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: WarningImHack3r
PR: WarningImHack3r/svelte-changelog#56
File: src/routes/package/+layout.server.ts:28-40
Timestamp: 2025-04-25T10:58:24.062Z
Learning: WarningImHack3r prefers elegant, aesthetically pleasing solutions for performance optimizations rather than quick fixes that introduce nested async IIFEs or similar approaches that might make code harder to read.
Learnt from: WarningImHack3r
PR: WarningImHack3r/svelte-changelog#48
File: src/routes/package/[...package]/+page.server.ts:50-53
Timestamp: 2025-04-11T10:33:07.002Z
Learning: WarningImHack3r prefers to keep server-side console.log statements in the codebase as they find them useful for debugging and monitoring, even if they might clean them up later.
🔇 Additional comments (6)
src/routes/tracker/[org]/[repo]/+page.svelte (6)
3-3: Clean import addition for Image componentThe import of
Imagefrom "@lucide/svelte" is correctly added to support rendering icons in the markdown snippet added later.
11-14: Well-structured type definition for ItemGood use of union types to include discussions alongside issues and PRs. The
NonNullabletype ensures type safety by preventing null values.
53-53: Improved type safety with genericsThe list snippet now properly uses generic typing with
T extends Item, which ensures type safety when rendering different item types.
74-78: Badge type detection for discussionsThe badge type logic has been extended to detect discussion items by checking for the presence of a
categoryproperty. This implementation correctly differentiates between PRs, issues, and discussions.
87-87: Safe property access for draft statusGood defensive programming practice to check for the existence of the
draftproperty before using it, preventing potential runtime errors.
132-139: Image rendering in markdownThe implementation provides a custom renderer for images in markdown, displaying an icon alongside the alt text.
Follow-up-ish of #69. This one adds support for discussions across the website, wherever issues and PRs already are.
More concretely, it adds support for them:
Discussions not being officially supported through the Octokit package, I'll have to create the types from other existing types. Thanks to an insightful comment, here are the endpoints to use:
As per the same comment:
/reactionsand/timelinedon't work from a singular discussion despite being linkedImportant
The (undocumented) API endpoint that lists all the discussions returns results in an ascending order, preventing the site from seeing the newest discussions.
Nothing works despite using the parameters documented for team discussions. I'll have to figure this out.Edit: figured it out (56e3b42).Roadmap
Details page
PR tracker
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores