feat: add browser extension to project #37
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis pull request introduces a comprehensive web browser extension for the Unsight.dev project, focusing on enhancing GitHub issue pages by displaying similar issues inline. The extension is built using Vue.js, Vite, and modern web technologies, with support for multiple browsers including Chrome and Firefox. It includes functionality to fetch and display related issues dynamically, along with configuration for development, testing, and deployment. Changes
Assessment against linked issues
Sequence DiagramsequenceDiagram
participant User
participant GitHubIssuePage
participant ContentScript
participant UnsightAPI
User->>GitHubIssuePage: Visits GitHub issue page
ContentScript->>UnsightAPI: Fetch similar issues
UnsightAPI-->>ContentScript: Return similar issues
ContentScript->>GitHubIssuePage: Inject similar issues section
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
nickytonline
force-pushed
the
web-ext
branch
from
a569b87 to
919fd16
Compare
|
Looks like Code Rabbit cleaned up some code from the web extension template in ea7dbdd. |
|
Added in 377ec78 |
|
@danielroe, I have this loading real data now. There are some URLs still hard-coded/cleanup still, but things are shaping up. We probably want to load issues though where the current issue is a part of that cluster. We probably need a different endpoint for that. Also, is it possible that an issue could be part of multiple clusters? Possibly? |
|
I've also noticed that Code Rabbit and the eslint rules in the project seem to be at odds as it keeps auto fixing files. |
nickytonline
force-pushed
the
web-ext
branch
from
c86fc68 to
6dddd3c
Compare
|
The autofix is caused by running unsight.dev/.github/workflows/autofix.yml Lines 30 to 31 in 15657bd You can ignore any formatting/linting suggestions that Coderabbit may give. To prevent autofix, just make sure to lint the code by running |
nickytonline
force-pushed
the
web-ext
branch
2 times, most recently
from
fff2e42 to
e7a8e9f
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: 27
🧹 Nitpick comments (28)
packages/webext/src/sidepanel/main.ts (2)
1-4: Consider adding file extension to styles importThe import statement for styles lacks a file extension, which might cause issues with certain bundlers or make it less clear what type of file is being imported.
-import '../styles' +import '../styles.css' // or appropriate extension (.scss, .less, etc.)
6-8: Add error handling for mount operationThe current implementation assumes the '#app' element always exists. Consider adding error handling to provide a better developer experience when the mount fails.
const app = createApp(App) setupApp(app) -app.mount('#app') +try { + app.mount('#app') +} catch (error) { + console.error('Failed to mount Vue application:', error) + // Consider adding telemetry or logging here +}packages/webext/vite.config.background.mts (2)
22-23: Consider security implications of source mapsThe current configuration allows source maps in development but disables them in production, which is good. However,
emptyOutDir: falsemight retain old build artifacts. Consider enablingemptyOutDirto ensure clean builds.- emptyOutDir: false, + emptyOutDir: true,
30-33: Consider adding content hashing for better cache controlThe static filename
index.mjsmight lead to caching issues during updates. Consider adding a content hash to the filename.output: { - entryFileNames: 'index.mjs', + entryFileNames: 'index.[hash].mjs', extend: true, },packages/webext/e2e/fixtures.ts (1)
42-48: Validate file I/O overhead in testing pipelines.Reading and parsing the manifest file for each test run introduces slight overhead. Though acceptable in most cases, consider caching its contents if you encounter performance issues in large test suites.
packages/webext/src/composables/useWebExtensionStorage.ts (1)
142-146: Assess performance implications of repeatedly processing storage changes.The listener applies a full read on every storage change event. If the extension processes frequent updates or large datasets, consider batching or selectively handling changes for improved performance.
packages/webext/.gitpod.Dockerfile (1)
6-7: Enhance Dockerfile with best practicesConsider these improvements:
- Pin the Firefox version for reproducible builds
- Clean up apt cache to reduce image size
- Combine RUN commands to reduce layers
Apply these modifications:
-RUN apt-get update \ - && apt-get install -y firefox +RUN apt-get update \ + && apt-get install -y firefox=* \ + && apt-get clean \ + && rm -rf /var/lib/apt/lists/*packages/webext/modules.d.ts (2)
1-7: Enhance type safety and documentation for Vue augmentationThe
$appinterface could benefit from:
- More specific typing for the context property
- JSDoc documentation explaining its purpose
- Validation for allowed context values
Consider this enhanced version:
/** * Augments Vue's runtime to provide application context. * @property {Object} $app - Global application context * @property {AppContext} $app.context - Indicates the current view context */ declare module '@vue/runtime-core' { type AppContext = 'sidebar' | 'popup' | 'options' interface ComponentCustomProperties { $app: { context: AppContext } } }
9-10: Consider adding comprehensive type declarationsThe empty export is correctly used to treat this as a module. However, consider expanding this file to include other extension-specific type declarations that might be needed across the codebase.
You might want to declare types for:
- Extension message formats
- Storage schema
- API responses for similar issues
packages/webext/src/popup/main.ts (1)
4-4: Consider adding type safety to styles importThe styles import could benefit from explicit typing to ensure type safety across the application.
Consider adding a type declaration:
declare module '../styles' { const styles: void export default styles }packages/webext/src/options/main.ts (1)
1-8: Consider creating a shared bootstrap utilityThis file is nearly identical to
popup/main.ts. Consider creating a shared bootstrap utility to reduce code duplication.Example implementation:
// src/utils/bootstrap.ts import { App, createApp } from 'vue' import { setupApp } from '~/logic/common-setup' import '../styles' export function bootstrapVueApp(RootComponent: Component) { const app = createApp(RootComponent) setupApp(app) app.mount('#app') return app }Then both popup and options could simply use:
import { bootstrapVueApp } from '~/utils/bootstrap' import App from './Options.vue' // or Popup.vue bootstrapVueApp(App)packages/webext/src/components/__tests__/Logo.test.ts (1)
5-11: Enhance test coverage for the Logo componentThe current test only verifies that the component renders without errors. Consider adding:
- Snapshot testing to track visual changes
- Specific assertions about logo structure and attributes
- Edge cases and negative scenarios
Here's a suggested enhancement:
describe('logo component', () => { it('should render', () => { const wrapper = mount(Logo) - expect(wrapper.html()).toBeTruthy() }) + + it('should match snapshot', () => { + const wrapper = mount(Logo) + expect(wrapper.element).toMatchSnapshot() + }) + + it('should have correct attributes', () => { + const wrapper = mount(Logo) + const img = wrapper.find('img') + expect(img.exists()).toBe(true) + expect(img.attributes('alt')).toBe('Logo') + }) })packages/webext/scripts/manifest.ts (1)
10-10: Consider making the script more modularThe immediate invocation of
writeManifest()makes the script less reusable and harder to test. Consider making this conditional or moving it to a separate entry point.-writeManifest() +if (require.main === module) { + writeManifest() + .catch((error) => { + console.error(error) + process.exit(1) + }) +}packages/webext/unocss.config.ts (1)
4-13: Consider enhancing UnoCSS configurationThe current configuration includes basic presets but could benefit from additional customisation for the browser extension context.
Consider adding:
export default defineConfig({ presets: [ presetUno(), presetAttributify(), presetIcons(), ], transformers: [ transformerDirectives(), ], + // Safelist for dynamically generated classes + safelist: [ + 'bg-primary', + 'text-secondary', + ], + // Custom shortcuts for common patterns + shortcuts: { + 'btn': 'px-4 py-2 rounded-lg', + 'card': 'bg-white shadow-md rounded-lg p-4', + }, + // Theme customisation + theme: { + colors: { + primary: '#4F46E5', + secondary: '#7C3AED', + }, + }, })packages/webext/shim.d.ts (1)
4-9: Brilliant implementation of type-safe protocols!The protocol definitions are well-structured and provide proper type safety. However, consider adding brief JSDoc comments for each protocol to document their specific use cases.
Consider adding documentation like this:
export interface ProtocolMap { // define message protocol types // see https://github.com/antfu/webext-bridge#type-safe-protocols + /** Protocol for handling previous tab information */ 'tab-prev': { title: string | undefined } + /** Protocol for retrieving current tab details */ 'get-current-tab': ProtocolWithReturn<{ tabId: number }, { title?: string }> }packages/webext/src/env.ts (1)
1-8: Consider making the forbidden protocols list more maintainableThe list of forbidden protocols is comprehensive, but consider moving it to a configuration file for easier maintenance and reusability across the extension.
Consider creating a separate config file:
// config/protocols.ts export const forbiddenProtocols = [ 'chrome-extension://', 'chrome-search://', // ... other protocols ] as const;packages/webext/playwright.config.ts (1)
6-15: Consider enhancing the test configuration for CI/CD environmentsWhilst the current configuration is suitable for local development, consider adding CI/CD-specific settings.
Consider these additions:
export default defineConfig({ testDir: './e2e', retries: 2, + timeout: 30000, // Add timeout for CI environments + workers: process.env.CI ? 1 : undefined, // Limit workers in CI webServer: { command: 'npm run dev', url: 'http://localhost:3303/popup/main.ts', reuseExistingServer: true, + timeout: 120000, // Add server startup timeout }, + use: { + trace: process.env.CI ? 'on-first-retry' : 'off', + }, })packages/webext/.gitpod.yml (1)
4-17: Consider adding error handling to task commandsThe task commands would benefit from error handling to ensure the development environment is properly initialised.
tasks: - init: pnpm install && pnpm run build name: dev command: | + set -e # Exit on error gp sync-done ready pnpm run dev - name: pnpm start:chromium command: | + set -e # Exit on error gp sync-await ready gp ports await 6080 gp preview $(gp url 6080)packages/webext/scripts/utils.ts (2)
6-6: Enhance path resolution robustnessThe path resolution utility could benefit from additional error handling and path normalisation.
-export const r = (...args: string[]) => resolve(__dirname, '..', ...args) +export const r = (...args: string[]) => { + const path = resolve(__dirname, '..', ...args) + // Ensure path doesn't escape base directory + if (!path.startsWith(resolve(__dirname, '..'))) + throw new Error('Path traversal detected') + return path +}
7-8: Add type assertions for environment variablesEnvironment variables should have proper type assertions to ensure type safety.
-export const isDev = process.env.NODE_ENV !== 'production' -export const isFirefox = process.env.EXTENSION === 'firefox' +export const isDev = process.env.NODE_ENV as string !== 'production' +export const isFirefox = (process.env.EXTENSION as string | undefined) === 'firefox'packages/webext/src/logic/common-setup.ts (2)
9-10: Enhance type safety for dependency injectionThe provided value should be typed to ensure type safety when using injection.
- app.provide('app', app.config.globalProperties.$app) + const APP_KEY = Symbol('app') + app.provide<AppGlobalProperties>(APP_KEY, app.config.globalProperties.$app)
12-14: Improve documentation for plugin installationThe comments about plugin installation could be more comprehensive with TypeScript examples.
/** * Install additional plugins for different contexts. * * @example * // Install for all contexts * app.use(i18n) * * @example * // Install excluding content-script context * if (context !== 'content-script') { * app.use(i18n, { * locale: 'en-GB', * messages: {...} * }) * } */packages/webext/src/background/contentScriptHMR.ts (1)
14-17: Enhance error handling and type safetyThe error handling could be more informative for debugging purposes. Additionally, the script injection lacks type safety for the error object.
Consider applying these improvements:
- }).catch(error => console.error(error)) + }).catch((error: unknown) => { + console.error('Failed to inject content script:', error) + if (error instanceof Error) { + console.error('Error details:', error.message) + } + })packages/webext/e2e/basic.spec.ts (1)
8-9: Consider making the selector more robustUsing dynamic IDs for selectors can be fragile. Consider using data attributes for more reliable test selectors.
- await page.locator(`#${name} button`).click() - await expect(page.locator(`#${name} h1`)).toHaveText('Vitesse WebExt') + await page.locator('[data-testid="extension-button"]').click() + await expect(page.locator('[data-testid="extension-heading"]')).toHaveText('Vitesse WebExt')packages/webext/src/contentScripts/views/App.vue (1)
19-24: Enhance loading and error statesThe current loading and error states lack detail and visual polish.
Consider enhancing the UI:
- <div v-if="status === 'loading'"> - Loading... - </div> - <div v-else-if="status === 'error'" className="text-red-700"> - Error loading similar issues - </div> + <div v-if="status === 'loading'" class="flex items-center gap-2"> + <div class="animate-spin h-4 w-4 border-2 border-gray-500 rounded-full" /> + <span>Loading similar issues...</span> + </div> + <div v-else-if="status === 'error'" class="p-2 rounded bg-red-50 text-red-700"> + <p class="font-medium">Failed to load similar issues</p> + <p class="text-sm">Please try again later or contact support if the issue persists.</p> + </div>packages/webext/src/background/main.ts (2)
16-21: Improve error handling for side panel setupThe error handling could be more specific and include proper error logging.
if (USE_SIDE_PANEL) { // @ts-expect-error missing types browser.sidePanel .setPanelBehavior({ openPanelOnActionClick: true }) - .catch((error: unknown) => console.error(error)) + .catch((error: unknown) => { + if (error instanceof Error) { + console.error('Failed to set side panel behaviour:', error.message) + } else { + console.error('Failed to set side panel behaviour:', error) + } + }) }
51-63: Add error type handling for tab retrievalThe error handling in the message handler could be more specific about the type of error encountered.
onMessage('get-current-tab', async () => { try { const tab = await browser.tabs.get(previousTabId) return { title: tab?.title, } } - catch { + catch (error) { + console.error('Failed to retrieve tab:', error instanceof Error ? error.message : error) return { title: undefined, } } })packages/webext/README.md (1)
96-111: Enhance development setup instructionsThe development setup instructions could be more detailed about browser-specific requirements and troubleshooting steps.
Add a section about browser-specific development requirements and common troubleshooting steps, particularly for Firefox's
web-extusage.🧰 Tools
🪛 LanguageTool
[uncategorized] ~110-~110: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...bash pnpm dev-firefoxweb-extauto reload the extension whenextension/files c...(AUTO_HYPHEN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
package.jsonis excluded by!package.json,!**/package.jsonpackages/webext/extension/assets/icon-512.pngis excluded by!**/*.pngpackages/webext/extension/assets/icon.svgis excluded by!**/*.svgpackages/webext/package.jsonis excluded by!**/package.jsonpackages/webext/src/assets/logo.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (45)
.gitignore(1 hunks)packages/webext/.gitpod.Dockerfile(1 hunks)packages/webext/.gitpod.yml(1 hunks)packages/webext/LICENSE(1 hunks)packages/webext/README.md(1 hunks)packages/webext/e2e/basic.spec.ts(1 hunks)packages/webext/e2e/fixtures.ts(1 hunks)packages/webext/extension/manifest.json(1 hunks)packages/webext/modules.d.ts(1 hunks)packages/webext/playwright.config.ts(1 hunks)packages/webext/scripts/manifest.ts(1 hunks)packages/webext/scripts/prepare.ts(1 hunks)packages/webext/scripts/utils.ts(1 hunks)packages/webext/shim.d.ts(1 hunks)packages/webext/src/auto-imports.d.ts(1 hunks)packages/webext/src/background/contentScriptHMR.ts(1 hunks)packages/webext/src/background/main.ts(1 hunks)packages/webext/src/components.d.ts(1 hunks)packages/webext/src/components/README.md(1 hunks)packages/webext/src/components/SimilarIssue.vue(1 hunks)packages/webext/src/components/__tests__/Logo.test.ts(1 hunks)packages/webext/src/composables/useWebExtensionStorage.ts(1 hunks)packages/webext/src/contentScripts/index.ts(1 hunks)packages/webext/src/contentScripts/views/App.vue(1 hunks)packages/webext/src/env.ts(1 hunks)packages/webext/src/global.d.ts(1 hunks)packages/webext/src/logic/common-setup.ts(1 hunks)packages/webext/src/manifest.ts(1 hunks)packages/webext/src/options/Options.vue(1 hunks)packages/webext/src/options/index.html(1 hunks)packages/webext/src/options/main.ts(1 hunks)packages/webext/src/popup/Popup.vue(1 hunks)packages/webext/src/popup/index.html(1 hunks)packages/webext/src/popup/main.ts(1 hunks)packages/webext/src/sidepanel/Sidepanel.vue(1 hunks)packages/webext/src/sidepanel/index.html(1 hunks)packages/webext/src/sidepanel/main.ts(1 hunks)packages/webext/src/styles/index.ts(1 hunks)packages/webext/src/styles/main.css(1 hunks)packages/webext/src/tests/demo.spec.ts(1 hunks)packages/webext/tsconfig.json(1 hunks)packages/webext/unocss.config.ts(1 hunks)packages/webext/vite.config.background.mts(1 hunks)packages/webext/vite.config.content.mts(1 hunks)packages/webext/vite.config.mts(1 hunks)
✅ Files skipped from review due to trivial changes (14)
- .gitignore
- packages/webext/src/options/Options.vue
- packages/webext/src/sidepanel/Sidepanel.vue
- packages/webext/LICENSE
- packages/webext/src/components/README.md
- packages/webext/src/components.d.ts
- packages/webext/src/styles/index.ts
- packages/webext/src/options/index.html
- packages/webext/src/popup/index.html
- packages/webext/src/styles/main.css
- packages/webext/src/sidepanel/index.html
- packages/webext/tsconfig.json
- packages/webext/src/popup/Popup.vue
- packages/webext/src/auto-imports.d.ts
🧰 Additional context used
🪛 LanguageTool
packages/webext/README.md
[uncategorized] ~21-~21: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...lang.org/) - type safe - 📦 Components auto importing - 🌟 [Icons](./src/c...
(AUTO_HYPHEN)
[uncategorized] ~110-~110: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...bash pnpm dev-firefox web-ext auto reload the extension when extension/ files c...
(AUTO_HYPHEN)
🪛 Markdownlint (0.37.0)
packages/webext/README.md
7-7: null
Images should have alternate text (alt text)
(MD045, no-alt-text)
9-9: null
Images should have alternate text (alt text)
(MD045, no-alt-text)
11-11: null
Images should have alternate text (alt text)
(MD045, no-alt-text)
🪛 Biome (1.9.4)
packages/webext/src/manifest.ts
[error] 87-87: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 83-83: Unexpected constant condition.
(lint/correctness/noConstantCondition)
🔇 Additional comments (9)
packages/webext/src/sidepanel/main.ts (1)
1-8: Verify the common setup implementation
The code looks good overall and follows Vue.js best practices. However, let's verify the implementation of the common setup function to ensure consistent configuration across the extension.
✅ Verification successful
Common setup implementation is consistent and properly used across extension contexts
The setupApp function in common-setup.ts provides a consistent configuration across all extension contexts (popup, sidepanel, options page, and content scripts). It:
- Injects a global
$appobject with context information - Provides app configuration via dependency injection
- Includes a designated location for installing additional plugins
The implementation is properly used in all relevant entry points:
- packages/webext/src/popup/main.ts
- packages/webext/src/sidepanel/main.ts
- packages/webext/src/options/main.ts
- packages/webext/src/contentScripts/index.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of common setup function
# and its usage across the codebase
# Look for the common-setup.ts file
echo "Searching for common-setup.ts implementation:"
rg -A 10 "export function setupApp"
# Check for other files using the same setup
echo "\nChecking usage of setupApp across the codebase:"
rg "setupApp\(app\)"
Length of output: 1366
packages/webext/vite.config.content.mts (1)
20-28: Verify Content Security Policy compatibility
The IIFE format is correct for content scripts, but ensure the output is compatible with the extension's Content Security Policy (CSP).
✅ Verification successful
Content Security Policy is correctly configured for IIFE bundle
The CSP in the manifest.json is properly configured:
- In production:
script-src 'self'; object-src 'self' - In development:
script-src 'self' http://localhost:${port}; object-src 'self'
The IIFE bundle from Vite will be served from the extension's own origin ('self'), making it fully compatible with the CSP configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for CSP in manifest files
rg -l "content_security_policy" | while read -r file; do
echo "=== CSP in $file ==="
cat "$file"
done
Length of output: 5777
packages/webext/vite.config.mts (2)
60-68: Consider edge cases for asset rewriting.
Rewriting asset paths via a regex might break in edge cases where nested directories or absolute paths are required. Test thoroughly for situations in which HTML files live in deeper or unexpected folder structures.
100-102: Note on disabling mangling.
Disabling code mangling can help debugging but results in larger bundle sizes. Confirm if the better readability justifies the potential impact on performance and bandwidth.
packages/webext/src/composables/useWebExtensionStorage.ts (1)
95-101: Check merging logic for unintended key overwrites.
Merging defaults into existing objects can overwrite keys unintentionally if they also appear in the saved data. Validate whether this behaviour aligns with user expectations or if a deeper merge strategy is desired.
packages/webext/src/popup/main.ts (1)
1-8: Well-structured Vue.js application setup
The application setup follows Vue.js best practices with proper separation of concerns. The use of a common setup function promotes code reuse across different parts of the extension.
packages/webext/src/global.d.ts (1)
1-3: Well-documented global constants
The global constants are properly typed and documented. The comment for NAME is particularly helpful for understanding its source.
packages/webext/unocss.config.ts (1)
1-2: Verify UnoCSS dependencies
Ensure all required UnoCSS dependencies are correctly specified in package.json.
✅ Verification successful
UnoCSS dependencies are properly configured
The required UnoCSS dependencies are correctly specified in package.json:
unocss(^0.62.2) - provides the core functionality including presets and transformers@unocss/reset(^0.62.2) - provides CSS reset utilities
Both packages are at the same version, which ensures compatibility between the components being imported and used in the configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all required UnoCSS dependencies are specified
jq -r '.dependencies + .devDependencies | keys[]' packages/webext/package.json | grep -E 'unocss|@unocss'
Length of output: 128
Script:
#!/bin/bash
# Check the versions of UnoCSS dependencies
jq -r '.dependencies["unocss"], .devDependencies["unocss"], .dependencies["@unocss/reset"], .devDependencies["@unocss/reset"]' packages/webext/package.json
Length of output: 183
packages/webext/src/env.ts (1)
14-14: Consider a more robust browser detection method
The current Firefox detection might be insufficient for all use cases. Consider using the browser.runtime.getBrowserInfo() API for more reliable detection.
Let's verify the browser detection usage:
| let previousTabId = 0 | ||
|
|
||
| // communication example: send previous tab title from background page | ||
| // see shim.d.ts for type declaration | ||
| browser.tabs.onActivated.addListener(async ({ tabId }) => { | ||
| if (!previousTabId) { | ||
| previousTabId = tabId | ||
| return | ||
| } | ||
|
|
||
| let tab: Tabs.Tab | ||
|
|
||
| try { | ||
| tab = await browser.tabs.get(previousTabId) | ||
| previousTabId = tabId | ||
| } | ||
| catch { | ||
| return | ||
| } | ||
|
|
||
| console.log('previous tab', tab) | ||
| sendMessage('tab-prev', { title: tab.title }, { context: 'content-script', tabId }) | ||
| }) |
There was a problem hiding this comment.
💡 Codebase verification
Memory leak risk: Add tab cleanup handler
The verification confirms that there's no cleanup mechanism for closed tabs. The previousTabId could potentially reference a closed tab, leading to unnecessary memory retention and failed browser.tabs.get() calls.
- Add a
browser.tabs.onRemovedevent listener to resetpreviousTabIdwhen the tracked tab is closed - Consider handling edge cases where the active tab is closed
🔗 Analysis chain
Consider memory management for tab tracking
The current implementation keeps track of only one previous tab. Consider implementing a cleanup mechanism for when tabs are closed.
Let me verify if there are any other tab-related event listeners or cleanup mechanisms in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for tab cleanup handling in the codebase
rg -A 5 "tabs.onRemoved" || echo "No tab cleanup handler found"
Length of output: 91
Script:
#!/bin/bash
# Search for any tab-related event listeners and cleanup
rg -A 5 "browser\.tabs\.(on|remove)" || echo "No tab event handlers found"
# Check if previousTabId is reset anywhere else
ast-grep --pattern 'previousTabId = 0'
# Look for any tab cleanup or management patterns
rg -A 5 "tab.*cleanup|cleanup.*tab"
Length of output: 477
| function generateIssueUrl(issue: Issue): string { | ||
| return new URL(`/${issue.owner}/${issue.repository}/issues/${issue.number}`, 'https://github.com').toString() | ||
| } |
There was a problem hiding this comment.
Add URL validation for issue links
The URL generation should include validation to prevent potential XSS attacks through malformed input.
function generateIssueUrl(issue: Issue): string {
+ const sanitizedOwner = encodeURIComponent(issue.owner)
+ const sanitizedRepo = encodeURIComponent(issue.repository)
+ const sanitizedNumber = encodeURIComponent(issue.number.toString())
- return new URL(`/${issue.owner}/${issue.repository}/issues/${issue.number}`, 'https://github.com').toString()
+ return new URL(`/${sanitizedOwner}/${sanitizedRepo}/issues/${sanitizedNumber}`, 'https://github.com').toString()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function generateIssueUrl(issue: Issue): string { | |
| return new URL(`/${issue.owner}/${issue.repository}/issues/${issue.number}`, 'https://github.com').toString() | |
| } | |
| function generateIssueUrl(issue: Issue): string { | |
| const sanitizedOwner = encodeURIComponent(issue.owner) | |
| const sanitizedRepo = encodeURIComponent(issue.repository) | |
| const sanitizedNumber = encodeURIComponent(issue.number.toString()) | |
| return new URL(`/${sanitizedOwner}/${sanitizedRepo}/issues/${sanitizedNumber}`, 'https://github.com').toString() | |
| } |
| <template> | ||
| <article class="flex flex-row gap-2 leading-tightest"> | ||
| <span class="flex-shrink-0 inline-block w-5 h-5 i-tabler-circle-dot text-green-500" /> | ||
| <div class="flex flex-row gap-2 flex-wrap md:flex-nowrap md:pb-6 flex-grow"> | ||
| <a | ||
| :href="generateIssueUrl(issue)" target="_blank" | ||
| class="line-clamp-1 flex-grow text-sm md:text-base lg:flex-grow-0 no-underline color-current hover:underline" | ||
| >{{ | ||
| issue.title | ||
| }}</a> | ||
| <div class="text-xs relative md:absolute md:mt-6 text-gray-700"> | ||
| <a | ||
| aria-current="page" :href="generateUnsightDotDevUrl(issue)" | ||
| class="no-underline hover:underline color-current" | ||
| >{{ | ||
| issue.repository }}</a> | ||
| · updated | ||
| <relative-time :datetime="issue.updated_at" /> · <a | ||
| :href="generateUnsightDotDevUrl(issue)" | ||
| class="no-underline hover:underline color-current" | ||
| >{{ | ||
| scoreAsPercentage(issue.score) }}% similar </a> | ||
| </div> | ||
| </div> | ||
| </article> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility features
The component could benefit from additional accessibility attributes.
- <article class="flex flex-row gap-2 leading-tightest">
+ <article class="flex flex-row gap-2 leading-tightest" role="listitem">
<span class="flex-shrink-0 inline-block w-5 h-5 i-tabler-circle-dot text-green-500" />
<div class="flex flex-row gap-2 flex-wrap md:flex-nowrap md:pb-6 flex-grow">
<a
:href="generateIssueUrl(issue)" target="_blank"
- class="line-clamp-1 flex-grow text-sm md:text-base lg:flex-grow-0 no-underline color-current hover:underline"
+ class="line-clamp-1 flex-grow text-sm md:text-base lg:flex-grow-0 no-underline color-current hover:underline"
+ :aria-label="'View issue: ' + issue.title"
>{{
issue.title
}}</a>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <template> | |
| <article class="flex flex-row gap-2 leading-tightest"> | |
| <span class="flex-shrink-0 inline-block w-5 h-5 i-tabler-circle-dot text-green-500" /> | |
| <div class="flex flex-row gap-2 flex-wrap md:flex-nowrap md:pb-6 flex-grow"> | |
| <a | |
| :href="generateIssueUrl(issue)" target="_blank" | |
| class="line-clamp-1 flex-grow text-sm md:text-base lg:flex-grow-0 no-underline color-current hover:underline" | |
| >{{ | |
| issue.title | |
| }}</a> | |
| <div class="text-xs relative md:absolute md:mt-6 text-gray-700"> | |
| <a | |
| aria-current="page" :href="generateUnsightDotDevUrl(issue)" | |
| class="no-underline hover:underline color-current" | |
| >{{ | |
| issue.repository }}</a> | |
| · updated | |
| <relative-time :datetime="issue.updated_at" /> · <a | |
| :href="generateUnsightDotDevUrl(issue)" | |
| class="no-underline hover:underline color-current" | |
| >{{ | |
| scoreAsPercentage(issue.score) }}% similar </a> | |
| </div> | |
| </div> | |
| </article> | |
| <template> | |
| <article class="flex flex-row gap-2 leading-tightest" role="listitem"> | |
| <span class="flex-shrink-0 inline-block w-5 h-5 i-tabler-circle-dot text-green-500" /> | |
| <div class="flex flex-row gap-2 flex-wrap md:flex-nowrap md:pb-6 flex-grow"> | |
| <a | |
| :href="generateIssueUrl(issue)" target="_blank" | |
| class="line-clamp-1 flex-grow text-sm md:text-base lg:flex-grow-0 no-underline color-current hover:underline" | |
| :aria-label="'View issue: ' + issue.title" | |
| >{{ | |
| issue.title | |
| }}</a> | |
| <div class="text-xs relative md:absolute md:mt-6 text-gray-700"> | |
| <a | |
| aria-current="page" :href="generateUnsightDotDevUrl(issue)" | |
| class="no-underline hover:underline color-current" | |
| >{{ | |
| issue.repository }}</a> | |
| · updated | |
| <relative-time :datetime="issue.updated_at" /> · <a | |
| :href="generateUnsightDotDevUrl(issue)" | |
| class="no-underline hover:underline color-current" | |
| >{{ | |
| scoreAsPercentage(issue.score) }}% similar </a> | |
| </div> | |
| </div> | |
| </article> |
packages/webext/README.md
Outdated
| <p align="center"> | ||
| <sub>Popup</sub><br/> | ||
| <img width="655" src="https://user-images.githubusercontent.com/11247099/126741643-813b3773-17ff-4281-9737-f319e00feddc.png"><br/> | ||
| <sub>Options Page</sub><br/> | ||
| <img width="655" src="https://user-images.githubusercontent.com/11247099/126741653-43125b62-6578-4452-83a7-bee19be2eaa2.png"><br/> | ||
| <sub>Inject Vue App into the Content Script</sub><br/> | ||
| <img src="https://user-images.githubusercontent.com/11247099/130695439-52418cf0-e186-4085-8e19-23fe808a274e.png"> | ||
| </p> |
There was a problem hiding this comment.
Add alt text to images for accessibility
The images in the documentation lack alt text, which is essential for accessibility.
<p align="center">
<sub>Popup</sub><br/>
-<img width="655" src="https://user-images.githubusercontent.com/11247099/126741643-813b3773-17ff-4281-9737-f319e00feddc.png"><br/>
+<img width="655" src="https://user-images.githubusercontent.com/11247099/126741643-813b3773-17ff-4281-9737-f319e00feddc.png" alt="Screenshot of the extension popup interface"><br/>
<sub>Options Page</sub><br/>
-<img width="655" src="https://user-images.githubusercontent.com/11247099/126741653-43125b62-6578-4452-83a7-bee19be2eaa2.png"><br/>
+<img width="655" src="https://user-images.githubusercontent.com/11247099/126741653-43125b62-6578-4452-83a7-bee19be2eaa2.png" alt="Screenshot of the extension options page"><br/>
<sub>Inject Vue App into the Content Script</sub><br/>
-<img src="https://user-images.githubusercontent.com/11247099/130695439-52418cf0-e186-4085-8e19-23fe808a274e.png">
+<img src="https://user-images.githubusercontent.com/11247099/130695439-52418cf0-e186-4085-8e19-23fe808a274e.png" alt="Screenshot showing Vue app injection into content script">
</p>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <p align="center"> | |
| <sub>Popup</sub><br/> | |
| <img width="655" src="https://user-images.githubusercontent.com/11247099/126741643-813b3773-17ff-4281-9737-f319e00feddc.png"><br/> | |
| <sub>Options Page</sub><br/> | |
| <img width="655" src="https://user-images.githubusercontent.com/11247099/126741653-43125b62-6578-4452-83a7-bee19be2eaa2.png"><br/> | |
| <sub>Inject Vue App into the Content Script</sub><br/> | |
| <img src="https://user-images.githubusercontent.com/11247099/130695439-52418cf0-e186-4085-8e19-23fe808a274e.png"> | |
| </p> | |
| <p align="center"> | |
| <sub>Popup</sub><br/> | |
| <img width="655" src="https://user-images.githubusercontent.com/11247099/126741643-813b3773-17ff-4281-9737-f319e00feddc.png" alt="Screenshot of the extension popup interface"><br/> | |
| <sub>Options Page</sub><br/> | |
| <img width="655" src="https://user-images.githubusercontent.com/11247099/126741653-43125b62-6578-4452-83a7-bee19be2eaa2.png" alt="Screenshot of the extension options page"><br/> | |
| <sub>Inject Vue App into the Content Script</sub><br/> | |
| <img src="https://user-images.githubusercontent.com/11247099/130695439-52418cf0-e186-4085-8e19-23fe808a274e.png" alt="Screenshot showing Vue app injection into content script"> | |
| </p> |
🧰 Tools
🪛 Markdownlint (0.37.0)
7-7: null
Images should have alternate text (alt text)
(MD045, no-alt-text)
9-9: null
Images should have alternate text (alt text)
(MD045, no-alt-text)
11-11: null
Images should have alternate text (alt text)
(MD045, no-alt-text)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/webext/src/global.d.ts (1)
5-8: 🛠️ Refactor suggestionStrengthen type safety for Vue components
Using
anytype for Vue components reduces TypeScript's effectiveness.Apply this improvement:
declare module '*.vue' { - const component: any + import type { DefineComponent } from 'vue' + const component: DefineComponent<{}, {}, any> export default component }packages/webext/src/contentScripts/views/App.vue (1)
13-17:⚠️ Potential issueEnhance URL parsing robustness
The current path parsing method is fragile and could break with URL variations.
Consider implementing a more robust URL parsing solution:
-const [, ownerPart, repoPart, , issuePart] = window.location.pathname.split('/') - -const issueUrl = computed(() => new URL(`/api/similarity/${ownerPart}/${repoPart}/${issuePart}`, VITE_UNSIGHT_DOT_DEV_BASE_URL).toString()) +function parseGitHubPath(pathname: string) { + const matches = pathname.match(/^\/([^/]+)\/([^/]+)\/issues\/(\d+)/) + if (!matches) { + throw new Error('Invalid GitHub issue URL') + } + return { + owner: matches[1], + repo: matches[2], + issue: matches[3], + } +} + +const { owner, repo, issue } = parseGitHubPath(window.location.pathname) +const issueUrl = computed(() => + new URL(`/api/similarity/${owner}/${repo}/${issue}`, VITE_UNSIGHT_DOT_DEV_BASE_URL).toString() +)
🧹 Nitpick comments (5)
packages/webext/README.md (3)
103-106: Enhance environment setup instructionsThe environment setup instructions could be more detailed:
- The
.env.examplefile location isn't specified- There's no mention of other potential environment variables
- The importance of the
VITE_UNSIGHT_DOT_DEV_BASE_URLisn't explainedConsider adding more context about the environment configuration.
129-135: Enhance build instructions with more detailsThe build instructions could be improved by:
- Explaining what files are generated during the build
- Providing instructions for generating .crx and .xpi files
- Adding links to the Chrome Web Store and Firefox Add-ons documentation for publishing
Consider expanding this section to make it more helpful for developers.
139-141: Add alt text and context for Volta
- The Volta image lacks alt text for accessibility
- The connection between Volta and this template could be better explained to provide more context for users
Consider adding alt text to the image and expanding on how this template relates to Volta.
packages/webext/src/global.d.ts (1)
1-3: Add documentation for the DEV constantThe NAME constant has proper documentation, but DEV lacks a description. Consider adding a JSDoc comment explaining its purpose.
/** Extension name, defined in packageJson.name */ +/** Boolean flag indicating development mode */ declare const __DEV__: boolean declare const __NAME__: stringpackages/webext/src/contentScripts/views/App.vue (1)
1-6: Document the purpose of uno.css importConsider adding a comment explaining why uno.css is required and its role in the component.
import SimilarIssue from '~/components/SimilarIssue.vue' +// Import UnoCSS styles for utility-first CSS framework import 'uno.css'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/webext/.env.example(1 hunks)packages/webext/README.md(1 hunks)packages/webext/src/components/SimilarIssue.vue(1 hunks)packages/webext/src/contentScripts/views/App.vue(1 hunks)packages/webext/src/global.d.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/webext/.env.example
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/webext/src/components/SimilarIssue.vue
🧰 Additional context used
🪛 LanguageTool
packages/webext/README.md
[uncategorized] ~21-~21: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...lang.org/) - type safe - 📦 Components auto importing - 🌟 [Icons](./src/c...
(AUTO_HYPHEN)
[uncategorized] ~117-~117: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...bash pnpm dev-firefox web-ext auto reload the extension when extension/ files c...
(AUTO_HYPHEN)
🪛 Markdownlint (0.37.0)
packages/webext/README.md
7-7: null
Images should have alternate text (alt text)
(MD045, no-alt-text)
9-9: null
Images should have alternate text (alt text)
(MD045, no-alt-text)
11-11: null
Images should have alternate text (alt text)
(MD045, no-alt-text)
🔇 Additional comments (4)
packages/webext/README.md (2)
5-12: Add alt text to images for accessibility
The images in the documentation lack alt text, which is essential for accessibility. This issue was previously identified and should be addressed.
🧰 Tools
🪛 Markdownlint (0.37.0)
7-7: null
Images should have alternate text (alt text)
(MD045, no-alt-text)
9-9: null
Images should have alternate text (alt text)
(MD045, no-alt-text)
11-11: null
Images should have alternate text (alt text)
(MD045, no-alt-text)
21-23: Verify relative paths in documentation
The links to components and icons directories use relative paths (./src/components) which might not work correctly in the GitHub UI. Consider using absolute URLs to ensure the links work consistently.
✅ Verification successful
Relative paths in documentation are correct and well-supported
The relative paths in the README.md are accurate and point to valid directories. The verification shows:
- The
./src/componentsdirectory exists and contains proper documentation - The components directory includes icon support through Iconify
- Content script functionality is present via
contentScriptHMR.ts
The documentation accurately reflects the project's structure and features, with working relative paths that are appropriate for both GitHub UI and local navigation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if the referenced directories exist
fd --type d "components" --base-directory packages/webext/src
Length of output: 73
Script:
#!/bin/bash
# Let's verify if there are any icon-related files in the components directory
rg -l "icon" packages/webext/src/components/
# Also check if there are any content script related files
fd "content" --type f packages/webext/src
Length of output: 178
Script:
#!/bin/bash
# Let's check the content of the components README to understand the structure
cat packages/webext/src/components/README.md
# Also check if there's any Vue-related content script setup
rg -l "vue.*content" packages/webext/src
Length of output: 534
🧰 Tools
🪛 LanguageTool
[uncategorized] ~21-~21: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...lang.org/) - type safe - 📦 Components auto importing - 🌟 [Icons](./src/c...
(AUTO_HYPHEN)
packages/webext/src/global.d.ts (1)
10-12: LGTM! Well-structured environment variable typing
The interface correctly defines the type for the environment variable following Vite's conventions.
packages/webext/src/contentScripts/views/App.vue (1)
7-11: LGTM! Robust environment variable validation
The code properly validates the required environment variable with a clear error message.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
packages/webext/README.md (6)
1-4: Consider expanding the introduction with additional contextThe introduction effectively describes the core functionality, but could benefit from:
- Mentioning the current development status and planned features
- Adding screenshots of the extension in action
- Including browser compatibility information
28-28: Fix grammar in environment setup instruction-When building the project locally copy the .env.example file to .env.local and fill in the required values: +When building the project locally, copy the .env.example file to .env.local and fill in the required values:🧰 Tools
🪛 LanguageTool
[uncategorized] ~28-~28: A comma might be missing here.
Context: ...xtension ``` When building the project locally copy the .env.example file to .env.loca...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
43-43: Correct grammar in web-ext description-`web-ext` auto reload the extension when `extension/` files changed. +`web-ext` auto-reloads the extension when `extension/` files change.🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...pm -F webext dev-firefox ```web-extauto reload the extension when `extension/` files c...(AUTO_HYPHEN)
13-46: Improve development instructions structureConsider reorganising the development section to:
- Environment setup (
.envconfiguration)- Development server options (clearly separated for different browsers)
- Hot reload information
- Troubleshooting tips
🧰 Tools
🪛 LanguageTool
[uncategorized] ~28-~28: A comma might be missing here.
Context: ...xtension ``` When building the project locally copy the .env.example file to .env.loca...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~43-~43: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...pm -F webext dev-firefox ```web-extauto reload the extension when `extension/` files c...(AUTO_HYPHEN)
55-55: Fix grammar in environment file instruction-ensure you have a .env file for the production build or pass the values as environment variables. +Ensure you have a .env file for the production build, or pass the values as environment variables.🧰 Tools
🪛 LanguageTool
[uncategorized] ~55-~55: Possible missing comma found.
Context: ...you have a .env file for the production build or pass the values as environment varia...(AI_HYDRA_LEO_MISSING_COMMA)
57-57: Fix grammar in extension store instruction-And then pack files under `extension`, you can upload `extension.crx` or `extension.xpi` to appropriate extension store. +And then pack files under `extension`, you can upload `extension.crx` or `extension.xpi` to the appropriate extension store.🧰 Tools
🪛 LanguageTool
[uncategorized] ~57-~57: You might be missing the article “the” here.
Context: ...dextension.crxorextension.xpito appropriate extension store. ## Credits Thanks to...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
packages/webext/src/contentScripts/views/App.vue (1)
21-39: Enhance accessibility and error handling in the templateThe current template could benefit from improved accessibility and error handling features.
- <details class="grid gap-2" aria-live="polite"> + <details class="grid gap-2" aria-live="polite" :open="issues && issues.length > 0"> <summary>Similar issues</summary> <div v-if="isFetching"> - Loading... + <span role="status">Loading similar issues...</span> </div> <div v-else-if="error" class="text-red-700"> - Error loading similar issues + <p>Failed to load similar issues</p> + <button + @click="refetch()" + class="text-sm underline hover:no-underline" + aria-label="Retry loading similar issues" + > + Retry + </button> </div> <template v-else-if="issues"> <p v-if="issues.length === 0"> No similar issues </p> - <ul v-else class="list-none grid gap-4" style="padding-inline-start: 0;"> + <ul + v-else + class="list-none grid gap-4" + style="padding-inline-start: 0;" + aria-label="List of similar issues" + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/webext/README.md(1 hunks)packages/webext/src/components/SimilarIssue.vue(1 hunks)packages/webext/src/contentScripts/index.ts(1 hunks)packages/webext/src/contentScripts/views/App.vue(1 hunks)packages/webext/src/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/webext/src/contentScripts/index.ts
- packages/webext/src/components/SimilarIssue.vue
🧰 Additional context used
🪛 LanguageTool
packages/webext/README.md
[uncategorized] ~28-~28: A comma might be missing here.
Context: ...xtension ``` When building the project locally copy the .env.example file to .env.loca...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~43-~43: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...pm -F webext dev-firefox ``` web-ext auto reload the extension when `extension/` files c...
(AUTO_HYPHEN)
[uncategorized] ~55-~55: Possible missing comma found.
Context: ...you have a .env file for the production build or pass the values as environment varia...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~57-~57: You might be missing the article “the” here.
Context: ...d extension.crx or extension.xpi to appropriate extension store. ## Credits Thanks to...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (3)
packages/webext/README.md (1)
59-61: LGTM!
The credits section is well-formatted and properly acknowledges the template source.
packages/webext/src/contentScripts/views/App.vue (2)
7-11: Move environment configuration to a separate file
Direct usage of environment variables makes testing and configuration management difficult.
13-15: Enhance path parsing robustness
The current path parsing implementation is fragile and could break with URL variations.
| ## Installation | ||
|
|
||
| From the root of the monorepo, run: | ||
|
|
||
| ```bash | ||
| pnpm install | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance installation instructions with prerequisites
Consider adding:
- Node.js version requirements
- pnpm installation instructions
- Repository cloning steps
- Development environment prerequisites
| ### Build | ||
|
|
||
| To build the extension, run | ||
|
|
||
| ```bash | ||
| pnpm -F webext build | ||
| ``` | ||
|
|
||
| ensure you have a .env file for the production build or pass the values as environment variables. | ||
|
|
||
| And then pack files under `extension`, you can upload `extension.crx` or `extension.xpi` to appropriate extension store. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance build instructions with additional details
Consider adding:
- Production environment variable requirements
- Extension packaging steps for different browsers
- Store submission guidelines
- Version bumping instructions
🧰 Tools
🪛 LanguageTool
[uncategorized] ~55-~55: Possible missing comma found.
Context: ...you have a .env file for the production build or pass the values as environment varia...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~57-~57: You might be missing the article “the” here.
Context: ...d extension.crx or extension.xpi to appropriate extension store. ## Credits Thanks to...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/webext/src/manifest.ts (2)
35-39: Optimise icons for different sizesCurrently using the same 512px icon for all sizes. Consider creating appropriately sized icons for better performance and visual quality.
icons: { - 16: './assets/icon-512.png', - 48: './assets/icon-512.png', - 128: './assets/icon-512.png', + 16: './assets/icon-16.png', + 48: './assets/icon-48.png', + 128: './assets/icon-128.png', },
81-83: Improve type safety for side panel configurationUsing
anytype assertion reduces type safety. Consider defining proper types for the manifest extension.- (manifest as any).side_panel = { + interface ChromiumManifest extends Manifest.WebExtensionManifest { + side_panel?: { default_path: string } + } + (manifest as ChromiumManifest).side_panel = {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.gitignore(1 hunks)packages/webext/src/manifest.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🪛 Biome (1.9.4)
packages/webext/src/manifest.ts
[error] 91-91: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 87-87: Unexpected constant condition.
(lint/correctness/noConstantCondition)
🔇 Additional comments (1)
packages/webext/src/manifest.ts (1)
6-7: 🛠️ Refactor suggestion
Consider tightening host permissions scope
The host permissions are quite broad. For production, consider restricting unsight.dev to specific API endpoints:
-const baseHostPermissions = ['https://github.com/*', 'https://unsight.dev/*']
+const baseHostPermissions = ['https://github.com/*', 'https://api.unsight.dev/*']Likely invalid or redundant comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
For some reason, in c070f9e, I can't access I've tried
I've also tried explicitly adding this to the vite config, but no difference. export default defineConfig({
envPrefix: 'VITE_',
// ...existing code...
})These are usually picked up by Vite, so not sure what's up. This is the only blocker at the moment, and I'm sure it's something super silly. |
|
sorry, got carried away! will merge and iterate in main... this looks great - thank you ❤️ |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (11)
packages/webext/README.md (3)
28-34: Improve grammar and clarity in development instructionsApply these improvements for better readability:
-When building the project locally copy the .env.example file to .env.local and fill in the required values: +When building the project locally, copy the .env.example file to .env.local and fill in the required values: -Then **load extension in browser with the `extension/` folder**. +Then **load the extension in the browser with the `extension/` folder**.🧰 Tools
🪛 LanguageTool
[uncategorized] ~28-~28: A comma might be missing here.
Context: ...xtension ``` When building the project locally copy the .env.example file to .env.loca...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~34-~34: You might be missing the article “the” here.
Context: ...http://localhost:3000" ``` Then **load extension in browser with theextension/folder...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~34-~34: You might be missing the article “the” here.
Context: ...ost:3000" ``` Then load extension in browser with theextension/folder. For Fi...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
13-46: Add browser-specific setup instructionsConsider enhancing the development section with:
- Step-by-step instructions for loading the extension in Chrome
- Step-by-step instructions for loading the extension in Firefox
- Screenshots of the browser extension settings pages
- Troubleshooting tips for common development issues
🧰 Tools
🪛 LanguageTool
[uncategorized] ~28-~28: A comma might be missing here.
Context: ...xtension ``` When building the project locally copy the .env.example file to .env.loca...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~34-~34: You might be missing the article “the” here.
Context: ...http://localhost:3000" ``` Then **load extension in browser with theextension/folder...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~34-~34: You might be missing the article “the” here.
Context: ...ost:3000" ``` Then load extension in browser with theextension/folder. For Fi...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~43-~43: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...pm -F webext dev-firefox ```web-extauto reload the extension when `extension/` files c...(AUTO_HYPHEN)
55-57: Improve grammar in build instructionsApply these improvements for better clarity:
-ensure you have a .env file for the production build or pass the values as environment variables. +Ensure you have a .env file for the production build, or pass the values as environment variables. -And then pack files under `extension`, you can upload `extension.crx` or `extension.xpi` to appropriate extension store. +And then pack files under `extension`, you can upload `extension.crx` or `extension.xpi` to the appropriate extension store.🧰 Tools
🪛 LanguageTool
[uncategorized] ~55-~55: Possible missing comma found.
Context: ...you have a .env file for the production build or pass the values as environment varia...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~57-~57: You might be missing the article “the” here.
Context: ...dextension.crxorextension.xpito appropriate extension store. ## Credits Thanks to...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
packages/webext/src/contentScripts/views/App.vue (2)
13-24: Enhance error handling with retriesWhilst the basic error handling is present, consider implementing a retry mechanism for transient failures.
const { data: issues, error, isFetching, } = useFetch<Issue[]>(issueUrl, { immediate: true, timeout: 5000, + retry: 3, + retryDelay: 1000, onFetchError: (ctx) => { console.error('Failed to fetch similar issues:', ctx.error) + // Add more specific error handling based on error type + if (ctx.error instanceof TypeError) { + console.error('Network error occurred') + } return ctx } }).json()
27-47: Brilliant use of semantic HTML and accessibility!The implementation shows excellent attention to accessibility with aria-live and semantic elements. Consider enhancing the loading state with more context.
- Loading... + <p class="loading-text"> + <span class="loading-spinner"></span> + Fetching similar issues... + </p>packages/webext/src/components/__tests__/SimilarIssue.spec.ts (1)
1-24: Consider extracting test data to fixturesThe test setup is well-structured, but moving the mock data to a separate fixture file would improve maintainability and reusability.
// __fixtures__/issues.ts export const mockIssue = { owner: 'nuxt', repository: 'nuxt', number: 26798, title: 'FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory', url: 'https://github.com/nuxt/nuxt/issues/26798', updated_at: '2024-12-12', labels: ['bug'], score: 0.87, }packages/webext/src/manifest.ts (4)
6-7: Consider making host permissions more specificThe host permissions could be more granular. For the unsight.dev domain, consider specifying only the required API endpoints rather than allowing access to the entire domain.
-const baseHostPermissions = ['https://github.com/*', 'https://unsight.dev/*'] +const baseHostPermissions = ['https://github.com/*', 'https://api.unsight.dev/*']
35-39: Consider using different icon sizesCurrently, the same icon file is used for all sizes. Consider providing appropriately sized icons for better visual quality and performance.
icons: { - 16: './assets/icon-512.png', - 48: './assets/icon-512.png', - 128: './assets/icon-512.png', + 16: './assets/icon-16.png', + 48: './assets/icon-48.png', + 128: './assets/icon-128.png', },
65-70: Strengthen content security policy in developmentThe development CSP could be more restrictive by specifying the exact script sources needed.
-? `script-src 'self' http://localhost:${port}; object-src 'self'` +? `script-src 'self' http://localhost:${port}/dist/; object-src 'self'`
79-84: Improve type safety for side panel configurationInstead of using
any, consider defining a proper type for the Chromium-specific manifest properties.+interface ChromiumManifest extends Manifest.WebExtensionManifest { + side_panel?: { + default_path: string; + }; +} - (manifest as any).side_panel = { + (manifest as ChromiumManifest).side_panel = {packages/webext/tests/e2e/basic.spec.ts (1)
1-2: Consider adding explicit type importsAdding type information for the imported utilities would improve code maintainability and IDE support.
-import { expect, isDevArtifact, name, test } from './fixtures' +import type { expect as ExpectType } from '@playwright/test' +import { expect, isDevArtifact, name, test } from './fixtures'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
package.jsonis excluded by!package.json,!**/package.jsonpackages/webext/extension/assets/icon-512.pngis excluded by!**/*.pngpackages/webext/extension/assets/icon.svgis excluded by!**/*.svgpackages/webext/package.jsonis excluded by!**/package.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (21)
.github/workflows/autofix.yml(1 hunks).github/workflows/ci.yml(1 hunks).gitignore(2 hunks)README.md(1 hunks)eslint.config.js(1 hunks)packages/webext/.gitignore(1 hunks)packages/webext/README.md(1 hunks)packages/webext/modules.d.ts(1 hunks)packages/webext/playwright.config.ts(1 hunks)packages/webext/scripts/manifest.ts(1 hunks)packages/webext/scripts/prepare.ts(1 hunks)packages/webext/src/background/main.ts(1 hunks)packages/webext/src/components/SimilarIssue.vue(1 hunks)packages/webext/src/components/__tests__/SimilarIssue.spec.ts(1 hunks)packages/webext/src/composables/useWebExtensionStorage.ts(1 hunks)packages/webext/src/contentScripts/index.ts(1 hunks)packages/webext/src/contentScripts/views/App.vue(1 hunks)packages/webext/src/manifest.ts(1 hunks)packages/webext/tests/e2e/basic.spec.ts(1 hunks)packages/webext/tests/e2e/fixtures.ts(1 hunks)packages/webext/vite.config.mts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/webext/.gitignore
🚧 Files skipped from review as they are similar to previous changes (11)
- README.md
- .gitignore
- packages/webext/modules.d.ts
- packages/webext/scripts/manifest.ts
- packages/webext/playwright.config.ts
- packages/webext/scripts/prepare.ts
- packages/webext/src/background/main.ts
- packages/webext/vite.config.mts
- packages/webext/src/composables/useWebExtensionStorage.ts
- packages/webext/src/contentScripts/index.ts
- packages/webext/src/components/SimilarIssue.vue
🧰 Additional context used
🪛 LanguageTool
packages/webext/README.md
[uncategorized] ~28-~28: A comma might be missing here.
Context: ...xtension ``` When building the project locally copy the .env.example file to .env.loca...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~34-~34: You might be missing the article “the” here.
Context: ...http://localhost:3000" ``` Then **load extension in browser with the extension/ folder...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~34-~34: You might be missing the article “the” here.
Context: ...ost:3000" ``` Then load extension in browser with the extension/ folder. For Fi...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~43-~43: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...pm -F webext dev-firefox ``` web-ext auto reload the extension when `extension/` files c...
(AUTO_HYPHEN)
[uncategorized] ~55-~55: Possible missing comma found.
Context: ...you have a .env file for the production build or pass the values as environment varia...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~57-~57: You might be missing the article “the” here.
Context: ...d extension.crx or extension.xpi to appropriate extension store. ## Credits Thanks to...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 Biome (1.9.4)
packages/webext/src/manifest.ts
[error] 91-91: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 87-87: Unexpected constant condition.
(lint/correctness/noConstantCondition)
🔇 Additional comments (8)
packages/webext/README.md (2)
5-11: Enhance installation instructions with prerequisites
Consider adding:
- Node.js version requirements
- pnpm installation instructions
- Repository cloning steps
- Development environment prerequisites
47-58: Enhance build instructions with additional details
Consider adding:
- Production environment variable requirements
- Extension packaging steps for different browsers
- Store submission guidelines
- Version bumping instructions
🧰 Tools
🪛 LanguageTool
[uncategorized] ~55-~55: Possible missing comma found.
Context: ...you have a .env file for the production build or pass the values as environment varia...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~57-~57: You might be missing the article “the” here.
Context: ...d extension.crx or extension.xpi to appropriate extension store. ## Credits Thanks to...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
packages/webext/src/contentScripts/views/App.vue (3)
1-6: Imports and setup look brilliant!
The imports are well-organised, with proper separation of types, utilities and components.
7-8: Move API URL to environment configuration
The hardcoded fallback URL makes it challenging to switch between environments.
9-11: Enhance path parsing robustness
The current path parsing implementation is brittle and could fail with URL variations.
.github/workflows/autofix.yml (1)
33-34: Brilliant addition of the unit test snapshot update step.
The inclusion of snapshot updates is well-placed. However, consider uncommenting the browser test snapshot updates as well, given that this PR introduces a browser extension.
Let's verify if there are any snapshot files that might need updating:
.github/workflows/ci.yml (2)
46-47: Splendid addition of the unit test step.
The placement after type checking is spot-on, ensuring type safety before running tests.
55-55: Verify Playwright noble image compatibility.
Whilst updating to Playwright v1.49.1 is good, the switch from focal to noble-based image warrants verification, as noble is relatively new.
Let's check the Playwright documentation and image availability:
✅ Verification successful
Let me gather more information about the Playwright configuration and usage in the codebase.
Noble-based Playwright image v1.49.1 is verified and safe to use
The verification shows that:
- The noble-based Playwright image v1.49.1 is officially available in Microsoft Container Registry
- The project uses Playwright v1.49.1 as a dev dependency
- The noble-based images have been consistently released since v1.45.0, indicating stability
- Both AMD64 and ARM64 architectures are supported
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the availability and stability of the Playwright noble image
# Check if the image exists and its tags
curl -s "https://mcr.microsoft.com/v2/playwright/tags/list" | jq '.tags[] | select(contains("noble"))'
# Look for Playwright-related configurations
fd -e config.ts -e config.js | rg -l 'playwright'
Length of output: 1087
Script:
#!/bin/bash
# Check for Playwright-related files and configurations
rg -l "playwright" --type yaml
rg -l "playwright" --type typescript
rg -l "playwright" --type javascript
# Look for package.json to check Playwright dependencies
fd "package.json" --type f --exec cat {} \; | jq -c 'select(.dependencies != null or .devDependencies != null) | {dependencies, devDependencies}'
Length of output: 1906
| @@ -4,6 +4,7 @@ import withNuxt from './packages/web/.nuxt/eslint.config.mjs' | |||
|
|
|||
| export default withNuxt(antfu()).append( | |||
| { | |||
| files: ['packages/web/**/*.ts'], | |||
There was a problem hiding this comment.
💡 Codebase verification
ESLint configuration should include TypeScript files from both packages
The verification confirms that TypeScript files exist in both packages/web/ (19 files) and packages/webext/ (24 files). The current ESLint configuration only covers the web package, leaving the webext TypeScript files unchecked. The configuration should be updated to include both packages:
- files: ['packages/web/**/*.ts'],
+ files: ['packages/{web,webext}/**/*.ts'],• Consider adding separate configurations if the two packages require different ESLint rules
• The console logging rule suggestion remains valid and should be environment-specific
🔗 Analysis chain
Verify the target directory for TypeScript files
The configuration targets TypeScript files in packages/web/**, but this PR introduces files in packages/webext/. Should this rule also apply to the browser extension's TypeScript files?
Let's verify the TypeScript file locations:
Additionally, consider making the console logging rules environment-specific:
files: ['packages/web/**/*.ts'],
rules: {
- 'no-console': 'off',
+ 'no-console': process.env.NODE_ENV === 'development' ? 'off' : 'warn',
},🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check TypeScript file locations to ensure correct ESLint coverage
# Find all TypeScript files in packages/web and packages/webext
echo "TypeScript files in packages/web:"
fd -e ts . packages/web || true
echo -e "\nTypeScript files in packages/webext:"
fd -e ts . packages/webext || true
Length of output: 1936
| expect(wrapper.html()).toMatchInlineSnapshot(` | ||
| "<article class="flex flex-row gap-2 leading-tightest"><span class="flex-shrink-0 inline-block w-5 h-5 i-tabler-circle-dot text-green-500"></span> | ||
| <div class="flex flex-row gap-2 flex-wrap md:flex-nowrap md:pb-6 flex-grow"><a href="https://github.com/nuxt/nuxt/issues/26798" target="_blank" class="issue-link"><span class="sr-only">View issue: </span>FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory</a> | ||
| <div class="text-xs relative md:absolute md:mt-6 text-gray-700"><a aria-current="page" href="https://unsight.dev/nuxt/nuxt" class="no-underline hover:underline color-current">nuxt</a> · updated <relative-time-stub datetime="2024-12-12"></relative-time-stub> · <a href="https://unsight.dev/nuxt/nuxt" class="no-underline hover:underline color-current">87.00% similar </a></div> | ||
| </div> | ||
| </article>" | ||
| `) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand test coverage with additional test cases
Whilst the snapshot test is a good start, consider adding tests for:
- Edge cases (empty labels, missing data)
- Prop validation
- Score formatting
- URL generation
it('should handle missing labels', () => {
const wrapper = mount(SimilarIssue, {
props: {
issue: {
...mockIssue,
labels: [],
},
},
global: {
stubs: ['relative-time'],
},
})
expect(wrapper.find('.labels').exists()).toBe(false)
})
it('should format score correctly', () => {
const wrapper = mount(SimilarIssue, {
props: {
issue: {
...mockIssue,
score: 0.8763,
},
},
global: {
stubs: ['relative-time'],
},
})
expect(wrapper.text()).toContain('87.63% similar')
})| permissions: [ | ||
| 'tabs', | ||
| 'storage', | ||
| 'activeTab', | ||
| 'sidePanel', | ||
| ], |
There was a problem hiding this comment.
💡 Codebase verification
Storage permission is required, but 'tabs' permission can be removed
The storage permission is actively used in the codebase through the useWebExtensionStorage composable (packages/webext/src/composables/useWebExtensionStorage.ts), which implements local storage functionality using the WebExtension storage API.
However, since 'activeTab' permission is already present, the 'tabs' permission can be removed as it's redundant and overly broad for the current implementation.
- Remove 'tabs' from the permissions array in packages/webext/src/manifest.ts
- Keep 'storage' as it's required for the storage functionality
- Keep 'activeTab' and 'sidePanel' as they are
🔗 Analysis chain
Review permission requirements
The extension requests broad permissions. Consider:
- Whether 'tabs' permission could be replaced with the more specific 'activeTab'.
- If 'storage' permission is actually needed for the current functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for storage API usage
rg -l "chrome\.storage|browser\.storage" packages/webext/
Length of output: 59
Script:
#!/bin/bash
# Search for storage API usage with broader pattern and context
rg -i "storage" packages/webext/ -B 2 -A 2
# Also check for any imports or requires related to storage
rg "(import|require).*storage" packages/webext/
# Check manifest.json if it exists
fd manifest.json packages/webext/ --exec cat {}
Length of output: 7969
| test('example test', async ({ page }, testInfo) => { | ||
| testInfo.skip(!isDevArtifact(), 'contentScript is in closed ShadowRoot mode') | ||
|
|
||
| await page.goto('https://example.com') | ||
|
|
||
| await page.locator(`#${name} button`).click() | ||
| await expect(page.locator(`#${name} h1`)).toHaveText('Vitesse WebExt') | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace example test with actual extension functionality tests
This test appears to be from a template and tests example.com functionality. Consider replacing it with meaningful tests that verify your extension's core features, such as:
- Similar issues panel injection
- Data fetching and display
- UI component rendering
| test('popup page', async ({ page, extensionId }) => { | ||
| await page.goto(`chrome-extension://${extensionId}/dist/popup/index.html`) | ||
| await expect(page.locator('button')).toHaveText('Open Options') | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance popup page test coverage
The current test only verifies button text. Consider adding tests for:
- Popup initialisation state
- User interactions
- Error states
- Navigation between popup views
| test('options page', async ({ page, extensionId }) => { | ||
| await page.goto(`chrome-extension://${extensionId}/dist/options/index.html`) | ||
| await expect(page.locator('img')).toHaveAttribute('alt', 'extension icon') | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand options page test coverage and verify extension URL handling
The test only checks the image alt text. Additionally, the hardcoded extension URL pattern might be fragile.
Consider:
- Adding tests for options page functionality
- Using a utility function for extension URLs:
function getExtensionUrl(extensionId: string, path: string) {
return `chrome-extension://${extensionId}/dist/${path}/index.html`;
}| context: async ({ headless }, use) => { | ||
| // workaround for the Vite server has started but contentScript is not yet. | ||
| await sleep(1000) | ||
| const context = await chromium.launchPersistentContext('', { | ||
| headless, | ||
| args: [ | ||
| ...(headless ? ['--headless=new'] : []), | ||
| `--disable-extensions-except=${extensionPath}`, | ||
| `--load-extension=${extensionPath}`, | ||
| ], | ||
| }) | ||
| await use(context) | ||
| await context.close() | ||
| }, |
There was a problem hiding this comment.
Replace fixed delay with proper server readiness check
Using a fixed delay (sleep) for server readiness is unreliable and could lead to flaky tests.
Consider implementing a proper readiness check:
async function waitForViteServer(maxAttempts = 10, interval = 100) {
for (let i = 0; i < maxAttempts; i++) {
try {
const response = await fetch('http://localhost:YOUR_PORT/health');
if (response.ok) return;
} catch {
await sleep(interval);
}
}
throw new Error('Vite server failed to start');
}| extensionId: async ({ context }, use) => { | ||
| // for manifest v3: | ||
| let [background] = context.serviceWorkers() | ||
| if (!background) | ||
| background = await context.waitForEvent('serviceworker') | ||
|
|
||
| const extensionId = background.url().split('/')[2] | ||
| await use(extensionId) | ||
| }, | ||
| }) | ||
|
|
||
| export const expect = test.expect | ||
|
|
||
| export function isDevArtifact() { | ||
| const manifest: Manifest.WebExtensionManifest = JSON.parse(fs.readFileSync(path.resolve(extensionPath, 'manifest.json'), 'utf-8')) | ||
| return Boolean( | ||
| typeof manifest.content_security_policy === 'object' | ||
| && manifest.content_security_policy.extension_pages?.includes('localhost'), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance extension ID extraction and manifest parsing robustness
The current implementation could be more resilient to edge cases and provide better type safety.
Consider these improvements:
interface DevManifest extends Manifest.WebExtensionManifest {
content_security_policy?: {
extension_pages?: string;
};
}
export async function getExtensionId(context: BrowserContext): Promise<string> {
const worker = await context.waitForEvent('serviceworker', {
timeout: 5000,
predicate: (worker) => worker.url().includes('chrome-extension://')
});
const match = worker.url().match(/chrome-extension:\/\/([^/]+)/);
if (!match?.[1]) throw new Error('Failed to extract extension ID');
return match[1];
}
export function isDevArtifact(): boolean {
try {
const manifest = JSON.parse(
fs.readFileSync(path.resolve(extensionPath, 'manifest.json'), 'utf-8')
) as DevManifest;
return Boolean(
manifest.content_security_policy?.extension_pages?.includes('localhost')
);
} catch (error) {
console.error('Failed to parse manifest:', error);
return false;
}
}| import type { Manifest } from 'webextension-polyfill' | ||
| import fs from 'node:fs' | ||
| import path from 'node:path' | ||
| import { setTimeout as sleep } from 'node:timers/promises' | ||
|
|
||
| import { test as base, type BrowserContext, chromium } from '@playwright/test' | ||
|
|
||
| export { name } from '../../package.json' | ||
|
|
||
| export const extensionPath = path.join(__dirname, '../../extension') | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add robust error handling for extension path resolution
The extension path setup lacks validation. Consider adding checks for directory existence and proper extension structure.
export const extensionPath = path.join(__dirname, '../../extension')
+if (!fs.existsSync(extensionPath)) {
+ throw new Error(`Extension directory not found: ${extensionPath}`);
+}
+if (!fs.existsSync(path.join(extensionPath, 'manifest.json'))) {
+ throw new Error(`manifest.json not found in extension directory`);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type { Manifest } from 'webextension-polyfill' | |
| import fs from 'node:fs' | |
| import path from 'node:path' | |
| import { setTimeout as sleep } from 'node:timers/promises' | |
| import { test as base, type BrowserContext, chromium } from '@playwright/test' | |
| export { name } from '../../package.json' | |
| export const extensionPath = path.join(__dirname, '../../extension') | |
| import type { Manifest } from 'webextension-polyfill' | |
| import fs from 'node:fs' | |
| import path from 'node:path' | |
| import { setTimeout as sleep } from 'node:timers/promises' | |
| import { test as base, type BrowserContext, chromium } from '@playwright/test' | |
| export { name } from '../../package.json' | |
| export const extensionPath = path.join(__dirname, '../../extension') | |
| if (!fs.existsSync(extensionPath)) { | |
| throw new Error(`Extension directory not found: ${extensionPath}`); | |
| } | |
| if (!fs.existsSync(path.join(extensionPath, 'manifest.json'))) { | |
| throw new Error(`manifest.json not found in extension directory`); | |
| } |
Yeah a bunch of boilerplate was still in there from the extension template. 😅 |
This PR introduces the unsight.dev browser extension to the project. The browser extension when installed, enhances GitHub issue pages with a Similar Issues section in the issue header section.
Note, don't be intimidated by the number of files. The majority of them are boilerplate from the web extension template.
The main files that are important in regards to functionality are currently:
I'm still quite new to Vue, so I'm probably holding it wrong. 😅 Any feedback there is very welcome.
Closes #4
For some reason, in c070f9e, I can't access
import.meta.env.VITE_UNSIGHT_DOT_DEV_BASE_URL, when it's set in a.envor.env.localfile.I've tried
VITE_UNSIGHT_DOT_DEV_BASE_URL,VUE_APP_UNSIGHT_DOT_DEV_BASE_URL, but they never get recognized, but the other vars underimport.meta.envdo, e.g.I've also tried explicitly adding this to the vite config, but no difference.
These are usually picked up by Vite, so not sure what's up. This is the only blocker at the moment, and I'm sure it's something super silly.
Screenshots & Recordings
Initial POC loading real data, although we probably want the clustered data where the current issue is part of that cluster.
Steps to get the browser extension working locally
gh co 37pnpm ito install the latest dependenciespnpm -F webext devto run the web extension project.chrome://extensions/. Enabled developer mode, then click the "Load Unpacked" button.packages/webext/extension