Feature: GitHub & Notion Preview Cards#586
Open
arifulhoque7 wants to merge 7 commits intoweDevsOfficial:developfrom
Open
Feature: GitHub & Notion Preview Cards#586arifulhoque7 wants to merge 7 commits intoweDevsOfficial:developfrom
arifulhoque7 wants to merge 7 commits intoweDevsOfficial:developfrom
Conversation
Add GitHub integration to preview issues and pull requests inline and manage a personal access token. - New routes for preview, batch-preview and test-connection (routes/github.php). - New GitHub_Preview_Controller to parse GitHub URLs, fetch single or batch previews from GitHub API, handle token/no-token requests, test connection, manage errors (access denied, rate limit), and cache responses (transient caching; successful responses cached 1 hour). - Persist github_access_token in Settings model and mask it in Settings_Transformer for safe display. - Add Vue components: github-preview-card (visual preview card), github-preview-container (extracts GitHub URLs and fetches previews), and settings/github (admin UI to view/change token and test connection). - Integrate previews into comments, tasks and discussions by stripping GitHub URLs from rendered HTML and inserting the preview container where appropriate. This provides authenticated and anonymous preview support, batch fetching, and UI for storing/masking the access token.
arifulhoque7
added
Needs Dev Review
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds GitHub and Notion preview support: new REST routes and controllers, settings masking and UI for tokens, Vue preview card/container components, mixin URL-stripping, component registration updates, and view changes to render previews for comments, tasks, and discussions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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: 6
🧹 Nitpick comments (5)
views/assets/src/components/common/github-preview-container.vue (3)
212-224: Same i18n issue in batch error handler.The error message should also be internationalized here for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@views/assets/src/components/common/github-preview-container.vue` around lines 212 - 224, The batch error handler sets a static English message "Could not fetch preview" into self.previews; change it to use the i18n translation helper so the message is localized (e.g. call this.$t or the component's $t method) when assigning error on each url in the error function inside the loop that populates self.previews (referenced alongside extractNumberFromUrl and extractRepoFromUrl); update the assignment to set error using the translation key (e.g. this.$t('github.preview.fetch_error')) so all previews use the localized message.
49-56: Consider debouncing content watch.If content changes rapidly (e.g., during typing in an editor), this could trigger many API calls. Consider adding a debounce or checking if URLs actually changed before fetching.
Proposed improvement
watch: { content: { - handler: function () { - this.fetchAllPreviews(); + handler: function ( newContent, oldContent ) { + // Only fetch if URLs actually changed + var newUrls = this.extractGitHubUrls( newContent ).map(function(u) { return u.url; }).join(','); + var oldUrls = this.extractGitHubUrls( oldContent ).map(function(u) { return u.url; }).join(','); + if ( newUrls !== oldUrls ) { + this.fetchAllPreviews(); + } }, immediate: false } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@views/assets/src/components/common/github-preview-container.vue` around lines 49 - 56, The content watcher (watch: { content: { handler: function () { this.fetchAllPreviews(); }, immediate: false } }) can fire many times; modify the content watcher to debounce calls to fetchAllPreviews (or only call when URLs actually change) by wrapping fetchAllPreviews in a debounced function (or compare new/old content inside the handler) so rapid edits don't trigger repeated API calls; update the watcher to call the debounced method (or conditional logic) instead of calling fetchAllPreviews directly.
163-171: Error message not internationalized.The error message
'Could not fetch preview'is hardcoded. For consistency with the rest of the application, consider using the__()translation function.Proposed fix
error: function () { self.$set( self.previews, url, { type: 'issue', number: self.extractNumberFromUrl( url ), state: 'error', repository: self.extractRepoFromUrl( url ), - error: 'Could not fetch preview', + error: __('Could not fetch preview', 'wedevs-project-manager'), html_url: url }); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@views/assets/src/components/common/github-preview-container.vue` around lines 163 - 171, Replace the hardcoded error string in the error callback of the GitHub preview setter with the i18n translation function; in the error handler inside github-preview-container.vue where self.$set(self.previews, url, { ... error: 'Could not fetch preview' ... }), change the value to use __('Could not fetch preview') (or this.__ if that is the component-scoped helper) so the message is internationalized, keeping the rest of the object (type, number via extractNumberFromUrl, repository via extractRepoFromUrl, html_url) unchanged.views/assets/src/components/settings/github.vue (1)
156-195: Consider consolidating settings requests.The
loadGitHubSettings()method makes two separate HTTP requests to load token and preview settings. Consider using a single request with multiple keys or accepting this minor inefficiency for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@views/assets/src/components/settings/github.vue` around lines 156 - 195, The loadGitHubSettings() method issues two requests (tokenRequest and previewsRequest); replace them with a single call using self.httpRequest that fetches both settings (either by requesting pm/v2/settings without a key to get all settings or by passing both keys if the API supports comma-separated keys), then iterate the returned data array once to set self.token_saved and self.masked_token (when item.key === 'github_access_token') and self.enable_previews (when item.key === 'github_enable_previews'), and remove the separate tokenRequest and previewsRequest objects so only one HTTP call is made via the existing httpRequest helper.src/GitHub/Controllers/GitHub_Preview_Controller.php (1)
217-239: Batch endpoint processes URLs sequentially.The
batch_previewmethod fetches each URL sequentially with a 10-second timeout per request. With up to 10 URLs, this could take up to 100 seconds in the worst case. The caching mechanism helps mitigate this for repeated requests.Consider documenting this limitation or reducing the batch limit if response times become problematic in practice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/GitHub/Controllers/GitHub_Preview_Controller.php` around lines 217 - 239, The batch_preview method currently processes URLs sequentially (using parse_github_url and fetch_github_data) causing up to 10 × timeout delays; change it to process the $urls in parallel instead of a foreach loop: keep the initial validation via parse_github_url, then build concurrent HTTP requests (e.g., using curl_multi_exec or a parallel HTTP client) to call the same logic as fetch_github_data for each valid $parsed entry, collect responses into $results and preserve the existing success/error shape; alternatively, if you prefer a simpler change, lower the array_slice limit from 10 to a smaller number to cap worst-case latency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/GitHub/Controllers/GitHub_Preview_Controller.php`:
- Around line 351-368: In the 429 handling block inside
GitHub_Preview_Controller (the branch checking if $status_code === 429), persist
the constructed rate-limited response to the shared cache (e.g., WP transient or
your existing caching layer) using a key derived from the request identity (for
example owner/repo/type/number or $parsed['url']) and a short TTL (e.g., 30–300
seconds), then return the cached payload; update the block that builds the array
with 'state' => 'rate_limited' to first set that array into cache so subsequent
calls hit the cache instead of GitHub until TTL expires.
In `@views/assets/src/components/common/github-preview-card.vue`:
- Line 2: The clickable card div currently only uses `@click`="openInGitHub" and
is inaccessible to keyboard users; make it focusable and operable via keyboard
by adding tabindex="0" and role="button" to the element and attach a
keydown/keyup handler (e.g., call openInGitHub when Enter or Space is pressed)
so keyboard activation triggers the same openInGitHub method; apply the same
change to the other instances referenced (lines 408-410) to keep behavior
consistent.
- Around line 130-166: Replace hardcoded user-facing strings in the computed
helpers with localized messages: in typeLabel, use the app i18n (e.g.,
this.$t('...')) for "Issue" and "Pull Request"; in stateLabel map states to
localized keys like this.$t('github.state.open'),
this.$t('github.state.closed'), this.$t('github.state.merged'); and in
relativeTime return localized relative-time strings (use i18n pluralization or a
localized format helper) instead of hardcoded English phrases—e.g.,
this.$t('time.years', {count: diffYears}) or this.$t('time.just_now')—so update
the functions typeLabel, stateLabel and relativeTime to call the localization
API and add appropriate translation keys for each label and time unit.
- Around line 18-20: The template directly accesses
previewData.repository.full_name which can throw if repository is missing;
update the github-preview-card.vue template (and any related computed/property
usage) to guard that access — e.g., use optional chaining or a small computed
getter (previewData.repository?.full_name or a getRepoName() computed that
returns previewData.repository ? previewData.repository.full_name : '') and
render a safe fallback (empty string or "Unknown repo") so the component won't
crash on partial error payloads.
- Around line 170-177: The openInGitHub method currently opens targetUrl
directly; validate it first by parsing targetUrl with the URL constructor
(catching exceptions), then ensure url.protocol === 'https:' and url.hostname
endsWith 'github.com' or endsWith 'githubusercontent.com' (or other allowed
GitHub subdomains you want), and only call window.open when validation passes;
if parsing/validation fails, do not open the window (optionally
return/console.warn) — update the openInGitHub function and its use of
this.previewData/this.url accordingly to perform this check before opening.
In `@views/assets/src/components/settings/github.vue`:
- Line 141: The data() initializer currently calls the mixin method
getSettings() to set enable_previews which can be unreliable; change data() to
initialize enable_previews to a safe default (e.g., true) and remove the direct
getSettings call, then in created() or mounted() call getSettings() or let
loadGitHubSettings() overwrite enable_previews (ensure loadGitHubSettings()
assigns to this.enable_previews) so the setting is populated after mixins are
available.
---
Nitpick comments:
In `@src/GitHub/Controllers/GitHub_Preview_Controller.php`:
- Around line 217-239: The batch_preview method currently processes URLs
sequentially (using parse_github_url and fetch_github_data) causing up to 10 ×
timeout delays; change it to process the $urls in parallel instead of a foreach
loop: keep the initial validation via parse_github_url, then build concurrent
HTTP requests (e.g., using curl_multi_exec or a parallel HTTP client) to call
the same logic as fetch_github_data for each valid $parsed entry, collect
responses into $results and preserve the existing success/error shape;
alternatively, if you prefer a simpler change, lower the array_slice limit from
10 to a smaller number to cap worst-case latency.
In `@views/assets/src/components/common/github-preview-container.vue`:
- Around line 212-224: The batch error handler sets a static English message
"Could not fetch preview" into self.previews; change it to use the i18n
translation helper so the message is localized (e.g. call this.$t or the
component's $t method) when assigning error on each url in the error function
inside the loop that populates self.previews (referenced alongside
extractNumberFromUrl and extractRepoFromUrl); update the assignment to set error
using the translation key (e.g. this.$t('github.preview.fetch_error')) so all
previews use the localized message.
- Around line 49-56: The content watcher (watch: { content: { handler: function
() { this.fetchAllPreviews(); }, immediate: false } }) can fire many times;
modify the content watcher to debounce calls to fetchAllPreviews (or only call
when URLs actually change) by wrapping fetchAllPreviews in a debounced function
(or compare new/old content inside the handler) so rapid edits don't trigger
repeated API calls; update the watcher to call the debounced method (or
conditional logic) instead of calling fetchAllPreviews directly.
- Around line 163-171: Replace the hardcoded error string in the error callback
of the GitHub preview setter with the i18n translation function; in the error
handler inside github-preview-container.vue where self.$set(self.previews, url,
{ ... error: 'Could not fetch preview' ... }), change the value to use __('Could
not fetch preview') (or this.__ if that is the component-scoped helper) so the
message is internationalized, keeping the rest of the object (type, number via
extractNumberFromUrl, repository via extractRepoFromUrl, html_url) unchanged.
In `@views/assets/src/components/settings/github.vue`:
- Around line 156-195: The loadGitHubSettings() method issues two requests
(tokenRequest and previewsRequest); replace them with a single call using
self.httpRequest that fetches both settings (either by requesting pm/v2/settings
without a key to get all settings or by passing both keys if the API supports
comma-separated keys), then iterate the returned data array once to set
self.token_saved and self.masked_token (when item.key === 'github_access_token')
and self.enable_previews (when item.key === 'github_enable_previews'), and
remove the separate tokenRequest and previewsRequest objects so only one HTTP
call is made via the existing httpRequest helper.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
routes/github.phpsrc/GitHub/Controllers/GitHub_Preview_Controller.phpsrc/Settings/Models/Settings.phpsrc/Settings/Transformers/Settings_Transformer.phpviews/assets/src/components/common/comments.vueviews/assets/src/components/common/github-preview-card.vueviews/assets/src/components/common/github-preview-container.vueviews/assets/src/components/project-discussions/individual-discussions.vueviews/assets/src/components/project-task-lists/list-comments.vueviews/assets/src/components/project-task-lists/single-task.vueviews/assets/src/components/project-task-lists/task-comments.vueviews/assets/src/components/settings/github.vueviews/assets/src/components/settings/header.vueviews/assets/src/components/settings/router.jsviews/assets/src/helpers/common-components.jsviews/assets/src/helpers/mixin/mixin.js
PHP: Cache and return 429 responses properly — store the 429 result in a transient before returning to avoid repeated rate-limit hits. Vue: Add keyboard accessibility (tabindex, role, Enter/Space handlers) and compute repo name via a computed prop. Localize UI strings and relative time using __(), _n() and sprintf for proper i18n. Add robust URL validation (URL parsing, https + github.com host check) with try/catch and early returns to avoid opening or throwing on invalid targets. Overall changes improve accessibility, localization, security and handling of rate-limited responses.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/GitHub/Controllers/GitHub_Preview_Controller.php (2)
67-68: Minor: Redundant condition and consider usingBearerprefix.The condition
$token && $token !== falseis redundant—if$tokenis truthy, it's already notfalse. Also, GitHub recommends using theBearerprefix for Personal Access Tokens, thoughtokenstill works.♻️ Proposed simplification
- if ( $token && $token !== false ) { - $headers['Authorization'] = 'token ' . $token; + if ( $token ) { + $headers['Authorization'] = 'Bearer ' . $token; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/GitHub/Controllers/GitHub_Preview_Controller.php` around lines 67 - 68, Simplify the redundant token check and use the recommended Bearer prefix: in GitHub_Preview_Controller (the code that sets $headers['Authorization']), replace the condition "if ($token && $token !== false)" with "if ($token)" and set the header value to use 'Bearer ' . $token instead of 'token ' . $token so $headers['Authorization'] = 'Bearer ' . $token;.
98-102: Minor: Redundant condition.
$token && ! empty( $token )is redundant—! empty()is always true when$tokenis truthy.♻️ Proposed simplification
- if ( $token && ! empty( $token ) ) { + if ( $token ) { $api_url = 'https://api.github.com/user'; } else { $api_url = 'https://api.github.com/rate_limit'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/GitHub/Controllers/GitHub_Preview_Controller.php` around lines 98 - 102, The if condition in the GitHub_Preview_Controller (the block checking $token and setting $api_url) uses a redundant test ($token && ! empty( $token )); replace it with a single emptiness check (e.g., ! empty($token)) to determine which API URL to set so the branch remains the same but the condition is simplified; update the if controlling the $api_url assignment accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/GitHub/Controllers/GitHub_Preview_Controller.php`:
- Around line 374-391: The error branch in GitHub_Preview_Controller that
returns on unexpected status codes (the check where $status_code !== 200 ||
empty($body)) must also persist the error result to cache to avoid repeated API
calls; update that block to build the same cache key used in the rate-limiting
branch (using $type, $number, $owner, $repo or the existing cache helper) and
store the returned payload (including 'state'=>'error' and the error message)
with the same TTL/mechanism as the rate-limit case (e.g., wp_cache_set or
set_transient via the project's cache helper) before returning it.
---
Nitpick comments:
In `@src/GitHub/Controllers/GitHub_Preview_Controller.php`:
- Around line 67-68: Simplify the redundant token check and use the recommended
Bearer prefix: in GitHub_Preview_Controller (the code that sets
$headers['Authorization']), replace the condition "if ($token && $token !==
false)" with "if ($token)" and set the header value to use 'Bearer ' . $token
instead of 'token ' . $token so $headers['Authorization'] = 'Bearer ' . $token;.
- Around line 98-102: The if condition in the GitHub_Preview_Controller (the
block checking $token and setting $api_url) uses a redundant test ($token && !
empty( $token )); replace it with a single emptiness check (e.g., !
empty($token)) to determine which API URL to set so the branch remains the same
but the condition is simplified; update the if controlling the $api_url
assignment accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/GitHub/Controllers/GitHub_Preview_Controller.phpviews/assets/src/components/common/github-preview-card.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- views/assets/src/components/common/github-preview-card.vue
When handling unexpected status codes or empty responses in GitHub_Preview_Controller, the constructed fallback result is now stored in a transient (set_transient with $cache_key) for 5 minutes and then returned. This avoids recomputing the same fallback on subsequent requests and reduces repeated processing/API calls.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Introduce Notion preview support and improve GitHub preview handling. Added routes/notion.php and a new Notion_Preview_Controller with single/batch preview and test-connection endpoints, parsing, API calls, caching, error handling and helper methods. Frontend: new notion-preview-card component, notion-preview-container integration, show Notion previews in comments, debounce content parsing, and add force_refresh to preview requests. Settings: persist notion_access_token and mask it in transformers. GitHub preview: better network/error messages and logging, JSON error handling, url-encoding for repo/owner, support force-refresh, reduced cache TTLs and UI/regex tweaks for more robust URL detection and warning display.
Normalize boolean setting checks in GitHub and Notion preview controllers using strict in_array (handles false, 'false', '0', 0). Make GitHub URL type detection case-insensitive for 'pull'. Rework GitHub response handling to detect rate limiting first (429 or 403 with x-ratelimit-remaining = 0), return a rate_limited state and message, and cache those results for 10 minutes. Handle access-denied/404/401 separately with an access_denied state and shorter (2 minutes) cache. Adjusted error messages accordingly.
arifulhoque7
changed the title
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (6)
src/Notion/Controllers/Notion_Preview_Controller.php (4)
239-254: Sequential batch processing may be slow.Each URL is fetched sequentially, which could take up to 100+ seconds in worst case (10 URLs × 10s timeout). This is likely acceptable for an initial implementation, but consider noting this as a potential future optimization using parallel requests (e.g.,
Requests::request_multiple()from WordPress HTTP API).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Notion/Controllers/Notion_Preview_Controller.php` around lines 239 - 254, The loop in the Notion preview handler processes $urls sequentially (using parse_notion_url and fetch_notion_data) which can be slow for up to 10 URLs; change the implementation to perform parallel HTTP calls (e.g., use WordPress Requests::request_multiple or another concurrent request utility) by first mapping $urls through parse_notion_url to build a request list, execute requests concurrently, then populate $results with success/error and pass responses into fetch_notion_data (or adapt fetch_notion_data to accept raw response bodies) so all fetches run in parallel instead of one-by-one.
72-74: Minor: Redundant condition check.The check
$token && $token !== falseis slightly redundant since a truthy$tokenalready implies it's notfalse. However, this is defensive coding and doesn't affect functionality.♻️ Optional simplification
- if ( $token && $token !== false ) { + if ( $token ) { $headers['Authorization'] = 'Bearer ' . $token; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Notion/Controllers/Notion_Preview_Controller.php` around lines 72 - 74, The condition in Notion_Preview_Controller that currently checks "if ($token && $token !== false)" is redundant; replace it with a single truthy check (e.g., "if ($token)") where the Authorization header is set (the block that assigns $headers['Authorization'] = 'Bearer ' . $token). This keeps the behavior identical while simplifying the conditional; ensure you update the check in the method that builds the request headers in Notion_Preview_Controller so only a single truthy $token test is used.
126-144: Consider handling rate limit (429) responses.The
test_connectionmethod handles 401 and generic non-200 statuses, but unliketry_fetch_pageandtry_fetch_database(which handle 429 explicitly), this method doesn't provide a specific message for rate limiting. Users may get a confusing generic error during rate limits.♻️ Proposed fix to add 429 handling
if ( $status_code === 401 ) { return [ 'success' => false, 'error' => __( 'Invalid token. Please check your Notion Internal Integration Token.', 'wedevs-project-manager' ), ]; } + if ( $status_code === 429 ) { + return [ + 'success' => false, + 'error' => __( 'Notion API rate limit exceeded. Please try again later.', 'wedevs-project-manager' ), + ]; + } + if ( $status_code !== 200 || empty( $body ) ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Notion/Controllers/Notion_Preview_Controller.php` around lines 126 - 144, The test_connection method currently checks $status_code for 401 and generic non-200 but misses explicit handling for 429 rate-limit responses; update Notion_Preview_Controller::test_connection to detect when $status_code === 429 and return the same structured response (array with 'success' => false and a clear rate-limit message) used by try_fetch_page/try_fetch_database, using $status_code/$body context and preserving the localization function __() and keys 'success' and 'error' so callers receive a specific "rate limit" error instead of the generic message.
490-582: Consider extracting shared logic betweentry_fetch_pageandtry_fetch_database.These two methods share approximately 80% identical code (error handling, caching, response parsing). A single parameterized method (e.g.,
try_fetch_resource($type, $uuid, $headers, $parsed)) could reduce duplication and make maintenance easier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Notion/Controllers/Notion_Preview_Controller.php` around lines 490 - 582, Both try_fetch_page and try_fetch_database duplicate most HTTP, error-handling, caching and response-parsing logic; refactor by extracting the shared flow into a new parameterized method try_fetch_resource($type, $uuid, $headers, $parsed) and have try_fetch_page and try_fetch_database call it with type='page' or 'database'. The new try_fetch_resource should perform wp_remote_get, WP_Error handling, status-code branches (404, 401/403 -> access_denied, 429 -> rate_limited), json decode + json_last_error check, build_error_result usage, transient caching keys (use md5($parsed['url']) like current cache_key), and return the common result structure; keep only resource-specific bits (e.g., extract_database_title / extract_icon / extract_cover / fetch_user_info and parent_type handling) in the callers or provide hooks/callbacks to transform the decoded $body into the final data payload. Update references to set_transient durations and ensure callers still sanitize/esc_url_raw fields as currently done.src/GitHub/Controllers/GitHub_Preview_Controller.php (2)
67-69: Minor: Simplify redundant token check.The condition
$token && $token !== falseis redundant since$tokenalone will short-circuit if falsy.♻️ Suggested simplification
- if ( $token && $token !== false ) { + if ( $token ) { $headers['Authorization'] = 'token ' . $token; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/GitHub/Controllers/GitHub_Preview_Controller.php` around lines 67 - 69, The if condition in GitHub_Preview_Controller that currently checks "if ( $token && $token !== false )" is redundant; simplify it to a single truthiness check for $token (e.g., use "if ($token)") before setting $headers['Authorization'] to 'token ' . $token so the behavior is unchanged but the expression is clearer and concise.
223-239: Consider request timeout for uncached batch scenarios.With a 10-URL limit and 10-second timeout per fetch, worst-case uncached requests could take 100+ seconds. In practice, caching mitigates this. However, if many URLs miss cache simultaneously (e.g., first load with multiple new GitHub links), the overall request may timeout depending on server configuration.
Consider adding early termination if cumulative time exceeds a threshold, or documenting the expected behavior for admins.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/GitHub/Controllers/GitHub_Preview_Controller.php` around lines 223 - 239, The current loop over $urls (after array_slice) calls parse_github_url and fetch_github_data for each entry and can exceed total request time when many uncached items are fetched; add a cumulative timeout guard by recording a start time (before the foreach) and after each fetch check elapsed time against a configurable threshold (e.g., $batchTimeoutSeconds) and if exceeded stop processing remaining URLs and mark them in $results with a timeout error; update the logic around parse_github_url and fetch_github_data to return early and ensure $results contains meaningful failure entries for skipped URLs so callers can handle partial results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/GitHub/Controllers/GitHub_Preview_Controller.php`:
- Around line 67-69: The if condition in GitHub_Preview_Controller that
currently checks "if ( $token && $token !== false )" is redundant; simplify it
to a single truthiness check for $token (e.g., use "if ($token)") before setting
$headers['Authorization'] to 'token ' . $token so the behavior is unchanged but
the expression is clearer and concise.
- Around line 223-239: The current loop over $urls (after array_slice) calls
parse_github_url and fetch_github_data for each entry and can exceed total
request time when many uncached items are fetched; add a cumulative timeout
guard by recording a start time (before the foreach) and after each fetch check
elapsed time against a configurable threshold (e.g., $batchTimeoutSeconds) and
if exceeded stop processing remaining URLs and mark them in $results with a
timeout error; update the logic around parse_github_url and fetch_github_data to
return early and ensure $results contains meaningful failure entries for skipped
URLs so callers can handle partial results.
In `@src/Notion/Controllers/Notion_Preview_Controller.php`:
- Around line 239-254: The loop in the Notion preview handler processes $urls
sequentially (using parse_notion_url and fetch_notion_data) which can be slow
for up to 10 URLs; change the implementation to perform parallel HTTP calls
(e.g., use WordPress Requests::request_multiple or another concurrent request
utility) by first mapping $urls through parse_notion_url to build a request
list, execute requests concurrently, then populate $results with success/error
and pass responses into fetch_notion_data (or adapt fetch_notion_data to accept
raw response bodies) so all fetches run in parallel instead of one-by-one.
- Around line 72-74: The condition in Notion_Preview_Controller that currently
checks "if ($token && $token !== false)" is redundant; replace it with a single
truthy check (e.g., "if ($token)") where the Authorization header is set (the
block that assigns $headers['Authorization'] = 'Bearer ' . $token). This keeps
the behavior identical while simplifying the conditional; ensure you update the
check in the method that builds the request headers in Notion_Preview_Controller
so only a single truthy $token test is used.
- Around line 126-144: The test_connection method currently checks $status_code
for 401 and generic non-200 but misses explicit handling for 429 rate-limit
responses; update Notion_Preview_Controller::test_connection to detect when
$status_code === 429 and return the same structured response (array with
'success' => false and a clear rate-limit message) used by
try_fetch_page/try_fetch_database, using $status_code/$body context and
preserving the localization function __() and keys 'success' and 'error' so
callers receive a specific "rate limit" error instead of the generic message.
- Around line 490-582: Both try_fetch_page and try_fetch_database duplicate most
HTTP, error-handling, caching and response-parsing logic; refactor by extracting
the shared flow into a new parameterized method try_fetch_resource($type, $uuid,
$headers, $parsed) and have try_fetch_page and try_fetch_database call it with
type='page' or 'database'. The new try_fetch_resource should perform
wp_remote_get, WP_Error handling, status-code branches (404, 401/403 ->
access_denied, 429 -> rate_limited), json decode + json_last_error check,
build_error_result usage, transient caching keys (use md5($parsed['url']) like
current cache_key), and return the common result structure; keep only
resource-specific bits (e.g., extract_database_title / extract_icon /
extract_cover / fetch_user_info and parent_type handling) in the callers or
provide hooks/callbacks to transform the decoded $body into the final data
payload. Update references to set_transient durations and ensure callers still
sanitize/esc_url_raw fields as currently done.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae3a0398-6a03-4849-abb4-dfa3619f6814
📒 Files selected for processing (2)
src/GitHub/Controllers/GitHub_Preview_Controller.phpsrc/Notion/Controllers/Notion_Preview_Controller.php
Introduce Loom video preview support: add backend Loom_Preview_Controller with single/batch preview endpoints and a test-connection route (transient caching, error/rate-limit handling, oEmbed fetching). Add routes/loom.php to register endpoints and respect loom_enable_previews setting. Add frontend Vue components (pm-loom-preview-card, pm-loom-preview-container) and a Loom settings view, update settings header and router/helpers/styles/mixins to surface the Loom tab and wire previews into comments, discussions and task descriptions. Batch requests are limited and previews default to enabled when unset.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Feature: GitHub & Notion Preview Cards
Branch:
feature/github-preview-cardsDate: 2026-03-06
Scope: 21+ files changed (new files + modifications), ~1,700 lines added
Overview
This feature adds rich preview cards for GitHub (issues/pull requests) and Notion (pages/databases) URLs found in task descriptions, task comments, list comments, and discussion threads. When a user pastes a GitHub or Notion URL, it is automatically detected, the raw URL is stripped from the visible text, and a styled card with live data from the respective API is rendered below the content.
New Files
Backend (PHP)
routes/github.phpgithub/preview,github/batch-preview,github/test-connectionroutes/notion.phpnotion/preview,notion/batch-preview,notion/test-connectionsrc/GitHub/Controllers/GitHub_Preview_Controller.phpsrc/Notion/Controllers/Notion_Preview_Controller.phpFrontend (Vue.js)
views/assets/src/components/common/github-preview-card.vueviews/assets/src/components/common/github-preview-container.vueviews/assets/src/components/common/notion-preview-card.vueviews/assets/src/components/common/notion-preview-container.vueviews/assets/src/components/settings/github.vueviews/assets/src/components/settings/notion.vueModified Files
Settings Infrastructure
src/Settings/Models/Settings.phpgithub_access_tokenandnotion_access_tokento the$hidden_settingsarray so tokens are treated as sensitive and never returned in raw form.src/Settings/Transformers/Settings_Transformer.phpgithub_access_tokenandnotion_access_tokenusing the existingmask_api_key()method (shows first/last few chars, masks the rest with*).Settings UI (Navigation & Routing)
views/assets/src/components/settings/header.vueviews/assets/src/components/settings/router.jsGitHubSettingsandNotionSettingscomponents.github→github_settings_tabandnotion→notion_settings_tab, both requiring admin capability.Global Component Registration
views/assets/src/helpers/common-components.jsGitHubPreviewContainerandNotionPreviewContainer.pm-github-previewandpm-notion-preview.Mixin (Shared Methods)
views/assets/src/helpers/mixin/mixin.jsstripGithubUrls(html)— strips GitHub issue/PR URLs (both<a>tags and plain text) from HTML content when GitHub previews are enabled. Cleans up empty<p>tags.stripNotionUrls(html)— same pattern for Notion URLs when Notion previews are enabled.Integration Points (5 Templates)
Each of these templates was modified to:
stripNotionUrls(stripGithubUrls(content))inv-htmlbindings<pm-github-preview>and<pm-notion-preview>components below the contentviews/assets/src/components/common/comments.vueviews/assets/src/components/project-task-lists/list-comments.vueviews/assets/src/components/project-task-lists/task-comments.vueviews/assets/src/components/project-task-lists/single-task.vueviews/assets/src/components/project-discussions/individual-discussions.vueCSS Layout Fix
views/assets/src/helpers/less/pm-style.lessflex-wrap: wrapto.pm-desc-contentso preview cards wrap to a new line.width: 100%to.pm-github-preview-containerand.pm-notion-preview-containerinside.pm-desc-contentfor full-width cards.views/assets/src/components/project-task-lists/single-task.vue(scoped styles)&:empty { display: none; }to.pm-task-descriptionto hide empty description div when URLs are fully stripped.Feature Details
GitHub Preview Cards
URL Detection:
https://github.com/{owner}/{repo}/(issues|pull)/{number}hrefattributesCard Display (issue/PR):
noopener,noreferrerAPI Integration:
api.github.com)Notion Preview Cards
URL Detection:
notion.soURLs containing a 32-char hex ID (with or without UUID dashes)/Page-Title-{id},/{workspace}/{id},/{id}?v={view_id}hrefattributesCard Display (page/database):
API Integration:
api.notion.com) with version header2022-06-28?v=parameter)Caching Strategy
Cache key format:
pm_github_preview_{md5(url)}/pm_notion_preview_{md5(url)}Batch Fetching
Settings Pages
Both GitHub and Notion settings pages follow the same pattern:
Token Management:
ghp_abc****xyz)Enable/Disable Toggle:
Connection Test:
URL Stripping
When previews are enabled, raw URLs are stripped from the rendered content to avoid duplication (URL shown as text + preview card below). The stripping:
<a>tags linking to GitHub/Notion URLs<p>tagsSecurity Measures
sanitize_text_field()on all text,esc_url_raw()on URLs,sanitize_hex_color_no_hash()on label colorsgithub.com/notion.soonly$hidden_settings, masked in API responses, never sent to client in fullnoopener,noreferreron allwindow.open()calls;v-htmlonly used with API-sourced data (not user HTML)sslverify: trueon allwp_remote_getcalls;rawurlencode()on path componentsAuthenticpermission; test-connection/settings requireSettings_Page_Access(admin only)WP_DEBUGis enabledjson_last_error()check afterjson_decodeto handle malformed API responsesReview Fixes Applied
During code review, the following issues were identified and fixed:
is_string()check on URL paramsrawurlencode()on owner/repo in GitHub API URLGitHub_Preview_Controller.phpgithub-preview-container.vuejson_last_error()checks after JSON decode/^(www\.)?github\.com$/pm-style.lesstypekeysNotion_Preview_Controller.php.pm-task-descriptiondiv when URLs strippedsingle-task.vueClose 291
Close 294
Close 295
Summary by CodeRabbit
New Features
Chores