feat(devtools): add local mode application#167
Open
lourencorodrigo wants to merge 2 commits intomainfrom
Open
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
lourencorodrigo
changed the title
lourencorodrigo
force-pushed
the
feat/devtools-server
branch
2 times, most recently
from
c8fc351 to
58d2fde
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nube-devtools/src/manifest.ts (1)
33-44:⚠️ Potential issue | 🔴 CriticalEmpty
matchesarray prevents script injection.The
web_accessible_resources.matchesis an empty array, which blocks all websites from accessinginject-extension-flag.js. However,chrome.scripting.registerContentScriptsinbackground/index.tsregisters this script to run in theMAINworld on all["http://*/*", "https://*/*"]pages. When a content script in theMAINworld injects a .js file, the web page must be able to access that resource—the emptymatchesarray contradicts this requirement and will cause the injection to fail at runtime.Specify the appropriate match patterns to allow web pages to access the injected script:
🔧 Suggested fix
web_accessible_resources: [ { resources: [ "img/logo-16.png", "img/logo-32.png", "img/logo-48.png", "img/logo-128.png", "inject-extension-flag.js", ], - matches: [], + matches: ["http://*/*", "https://*/*"], }, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nube-devtools/src/manifest.ts` around lines 33 - 44, The web_accessible_resources entry currently has an empty matches array which prevents the page from loading inject-extension-flag.js when the background script (see background/index.ts registerContentScripts) injects it into the MAIN world; update the web_accessible_resources object in manifest.ts (the web_accessible_resources property that lists "inject-extension-flag.js") to include appropriate host match patterns (e.g. "http://*/*" and "https://*/*" or the equivalent allowed patterns) so pages can access the injected script at runtime.
🧹 Nitpick comments (6)
packages/nube-devtools/src/hooks/use-nube-status.ts (1)
3-14: Consider adding a guard forchrome.tabsconsistency.Line 5 checks
typeof chrome !== "undefined"before accessingchrome.devtools, but line 11 directly callschrome.tabs.querywithout a similar guard. While this is unlikely to fail in an extension context, adding consistency would improve robustness.♻️ Optional: Add consistent guard
const getTabId = (callback: (tabId: number | null) => void) => { if ( typeof chrome !== "undefined" && chrome.devtools?.inspectedWindow?.tabId != null ) { callback(chrome.devtools.inspectedWindow.tabId); return; } + if (typeof chrome === "undefined" || !chrome.tabs) { + callback(null); + return; + } chrome.tabs.query({ active: true, currentWindow: true }, (tabs) => { callback(tabs[0]?.id ?? null); }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nube-devtools/src/hooks/use-nube-status.ts` around lines 3 - 14, The getTabId helper accesses chrome.tabs.query without checking chrome or chrome.tabs; update getTabId to first guard that chrome and chrome.tabs exist (similar to the existing check for chrome.devtools.inspectedWindow.tabId) before calling chrome.tabs.query, and if absent call callback(null) (or otherwise return) so the function safely handles non-extension environments; ensure references to chrome.devtools.inspectedWindow.tabId, chrome.tabs.query, and the callback parameter remain intact.packages/nube-devtools/src/popup/app.tsx (1)
6-14: Consider a loading state to avoid flash of Unavailable page.Since
useNubeStatusinitializes tofalseand updates asynchronously, users may briefly see theUnavailablepage before the status check completes. Consider adding a loading state for better UX.This is a minor UX polish that can be addressed later if desired.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nube-devtools/src/popup/app.tsx` around lines 6 - 14, AppContent currently renders Unavailable immediately because useNubeStatus initializes to false; change the render logic to handle a loading/unknown state so the Unavailable screen doesn't flash: update AppContent to treat a third "loading" value (e.g., undefined or null) from useNubeStatus or add a local loading boolean and render a Loading/Spinner or nothing while status is pending, then render Unavailable only when status is definitively false and Popup when true; refer to AppContent, useNubeStatus, Unavailable, and Popup when making the change.packages/nube-devtools/src/background/index.ts (1)
21-24: Broad error suppression may hide real issues.The
.catch(() => {})silently ignores all errors, not just the expected "already registered" error. This could mask genuine issues like permission problems or invalid script paths.♻️ Consider checking for expected error
chrome.scripting .registerContentScripts([ { id: "nube-devtools-extension-flag", matches: ["http://*/*", "https://*/*"], js: ["inject-extension-flag.js"], runAt: "document_start", world: "MAIN", }, ]) - .catch(() => { - // Already registered (e.g. service worker restart) — safe to ignore. - }); + .catch((err) => { + // Duplicate ID error is expected on service worker restart — safe to ignore. + if (!err?.message?.includes("Duplicate script ID")) { + console.error("Failed to register extension flag script:", err); + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nube-devtools/src/background/index.ts` around lines 21 - 24, Replace the broad silent .catch(() => {}) on the service worker registration promise with a handler that inspects the thrown error (capture it as err) and only suppresses the known harmless "already registered" condition (e.g., check err.message or err.name for the expected text), otherwise surface unexpected failures by logging or rethrowing (for example: if err.message.includes('already registered') return; else console.error('Service worker registration failed', err)). This targets the registration promise/error handling and avoids hiding real issues while preserving the original intent to ignore benign "already registered" errors.packages/nube-devtools/src/pages/local-mode-content.tsx (1)
24-25: Extract the storage key into a shared constant module.
PAGE_STORAGE_KEY_DEVTOOLS_APPLICATIONis duplicated across modules (including content script). Centralizing it avoids silent drift bugs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nube-devtools/src/pages/local-mode-content.tsx` around lines 24 - 25, Extract the duplicated PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION constant into a single shared constants module and import it where needed instead of redefining it; update the declaration in local-mode-content.tsx (the const PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION) to import the shared constant, and remove the duplicate definitions from other modules (including the content script) so all code references use the centralized export (e.g., export const PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION) to prevent drift.packages/nube-devtools/src/utils/page-storage.ts (1)
18-22: Consider extracting active-tab resolution to a shared helper.The same active-tab query/guard pattern appears three times; pulling it into one helper will reduce duplication and keep behavior consistent.
Refactor sketch
+async function getActiveTabId(): Promise<number | null> { + const [tab] = await chrome.tabs.query({ active: true, currentWindow: true }); + return tab?.id ?? null; +} + export async function setPageSessionStorage( key: string, value: string, ): Promise<boolean> { - const [tab] = await chrome.tabs.query({ - active: true, - currentWindow: true, - }); - if (!tab?.id) return false; + const tabId = await getActiveTabId(); + if (tabId == null) return false; try { await chrome.scripting.executeScript({ - target: { tabId: tab.id }, + target: { tabId }, world: "MAIN", func: (k: string, v: string) => { sessionStorage.setItem(k, v); }, args: [key, value], });Also applies to: 46-50, 76-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nube-devtools/src/utils/page-storage.ts` around lines 18 - 22, Extract the repeated active-tab resolution into a shared helper (e.g., async function getActiveTabId()) that wraps chrome.tabs.query({ active: true, currentWindow: true }) and returns the tab id or null/false; replace the three duplicate patterns (the one that queries chrome.tabs in page-storage.ts at the three locations) with calls to getActiveTabId() and guard early when it returns no id. Ensure the helper lives alongside other utilities in the same module and update all call sites to use the helper instead of duplicating the chrome.tabs.query + if (!tab?.id) return false pattern.packages/nube-devtools/src/contentScript/index.ts (1)
34-37: Remove the commented-out initialization block.Line [34] through Line [37] are dead commented code and make the focus handler noisier than needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nube-devtools/src/contentScript/index.ts` around lines 34 - 37, Remove the dead commented initialization block for the runtime message — delete the commented chrome.runtime.sendMessage({...}) lines so the focus handler in index.ts is not cluttered; specifically remove the commented block containing "chrome.runtime.sendMessage({ action: \"nube-devtools-initialize-sdk\" });" near the end of the focus handler/closing brace to leave only active code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/nube-devtools/src/devtools/app.tsx`:
- Line 13: The Unavailable guard currently returns <Unavailable /> whenever
nubeStatus is falsy, which prevents the Local Mode flow; update the guard logic
so LocalModePage (route PAGES.LOCAL_MODE) is allowed through even if nubeStatus
is falsy — e.g., check the current route or page first and render LocalModePage
when PAGES.LOCAL_MODE before the nubeStatus short-circuit, or change the
condition to if (!nubeStatus && currentPage !== PAGES.LOCAL_MODE) return
<Unavailable /> so LocalModePage (LocalModePage / PAGES.LOCAL_MODE) is not
blocked.
In `@packages/nube-devtools/src/devtools/index.css`:
- Around line 129-137: Add a blank line before the scrollbar-width declaration
to satisfy Stylelint formatting rules: locate the base layer rule where the
universal selector (*) sets properties (see the block containing `@apply`
border-border outline-ring/50 and the scrollbar-width declaration) and insert an
empty line before the scrollbar-width: thin; declaration (and mirror formatting
consistency for the .dark * block with scrollbar-color if needed).
In `@packages/nube-devtools/src/pages/local-mode-content.tsx`:
- Around line 40-57: The async checkBundle function can suffer from stale
responses overwriting current state; introduce a per-request identity (e.g., a
requestId counter stored in a useRef) that you increment and capture at the
start of checkBundle, then before any state updates (setStatus, setIsChecking)
verify the captured id still matches the latest ref value—only then apply the
state changes; implement the same request-id guard for the other async
bundle/dev-server check in this file that also does a fetch so older responses
cannot override newer URL checks.
- Around line 139-153: The code currently ignores the boolean result from
setPageSessionStorage(PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION,
JSON.stringify(data)); — update the flow in the function that calls
setPageSessionStorage so you await the call, check its returned success value,
and only proceed to setIsConnecting(false), setStatus("connected"),
chrome.runtime.sendMessage({ action: "nube-devtools-app-server-status", payload:
{ connected: true } }) and await reloadCurrentTab() when the write succeeded; on
failure, log or surface the error, set an error/failed status (e.g.,
setStatus("error") or keep disconnected), setIsConnecting(false), and send an
appropriate disconnected/error runtime message instead of declaring connected.
- Around line 157-169: The stop flow currently calls JSON.parse on persisted
data (via getPageSessionStorage and PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION)
without guarding against malformed JSON; wrap the parse in a try/catch inside
the handleStop logic so a JSON parse error doesn’t abort cleanup: if parsing
fails, treat the stored value as absent or create a fresh object, ensure you
still call setPageSessionStorage to clear or set connected=false as appropriate,
and swallow/log the parse error (use the same logger/context used elsewhere) so
the rest of the stop/reload sequence proceeds reliably.
In `@packages/nube-devtools/src/popup/index.css`:
- Around line 138-141: The Biome formatter failed on the CSS block containing
the "body" selector (the rule with "min-width: 23rem;" and "margin: 0;"); run
the Biome formatter (biome format) or adjust spacing so the block follows
project style (proper indentation and spacing) and re-run CI. Specifically, open
the popup index.css rule for body, ensure consistent whitespace and newline
placement around the declarations, save, and commit the formatted file so the
Biome check passes.
- Around line 69-84: There are duplicate sidebar CSS variable declarations: the
oklch set (--sidebar, --sidebar-foreground, --sidebar-primary,
--sidebar-primary-foreground, --sidebar-accent, --sidebar-accent-foreground,
--sidebar-border, --sidebar-ring) is declared first and then redeclared in HSL
later causing unintended overrides; remove the second HSL block so each
--sidebar* variable is defined only once (keep the oklch definitions) and verify
no duplicate --sidebar* variables remain in the file.
- Around line 111-126: The dark theme block duplicates sidebar CSS custom
properties (e.g., --sidebar, --sidebar-foreground, --sidebar-primary,
--sidebar-primary-foreground, --sidebar-accent, --sidebar-accent-foreground,
--sidebar-border, --sidebar-ring, --sidebar-background) using HSL-like values
that override the earlier OKLCH declarations; remove the duplicate HSL-style
declarations (the second set) so only the intended OKLCH sidebar variables
remain, mirroring the fix applied to the light theme and ensuring no duplicate
custom-property declarations exist for those --sidebar* names.
In `@packages/nube-devtools/src/popup/popup.tsx`:
- Around line 22-25: The main element wrapping LocalModeContent is described as
"scrollable" but lacks an overflow class, so content can be clipped; update the
<main> element's className (the one containing "flex-1 flex justify-center p-3")
to include an appropriate overflow utility (e.g., overflow-auto or
overflow-y-auto) so the LocalModeContent can scroll when it exceeds popup height
constraints.
In `@packages/nube-devtools/src/utils/page-storage.ts`:
- Around line 11-12: The JSDoc for setPageSessionStorage claims non-strings are
stringified but the function signature and implementation only accept and pass a
string; update the contract by either (A) changing the JSDoc to state value must
be a string, or (B) change setPageSessionStorage to accept value: unknown (or
any) and stringify non-string values before sending to the page script (ensure
the injected script uses sessionStorage.setItem with the stringified value).
Locate the setPageSessionStorage function and the code that constructs the
script payload to implement the chosen fix so docs and runtime behavior match.
---
Outside diff comments:
In `@packages/nube-devtools/src/manifest.ts`:
- Around line 33-44: The web_accessible_resources entry currently has an empty
matches array which prevents the page from loading inject-extension-flag.js when
the background script (see background/index.ts registerContentScripts) injects
it into the MAIN world; update the web_accessible_resources object in
manifest.ts (the web_accessible_resources property that lists
"inject-extension-flag.js") to include appropriate host match patterns (e.g.
"http://*/*" and "https://*/*" or the equivalent allowed patterns) so pages can
access the injected script at runtime.
---
Nitpick comments:
In `@packages/nube-devtools/src/background/index.ts`:
- Around line 21-24: Replace the broad silent .catch(() => {}) on the service
worker registration promise with a handler that inspects the thrown error
(capture it as err) and only suppresses the known harmless "already registered"
condition (e.g., check err.message or err.name for the expected text), otherwise
surface unexpected failures by logging or rethrowing (for example: if
err.message.includes('already registered') return; else console.error('Service
worker registration failed', err)). This targets the registration promise/error
handling and avoids hiding real issues while preserving the original intent to
ignore benign "already registered" errors.
In `@packages/nube-devtools/src/contentScript/index.ts`:
- Around line 34-37: Remove the dead commented initialization block for the
runtime message — delete the commented chrome.runtime.sendMessage({...}) lines
so the focus handler in index.ts is not cluttered; specifically remove the
commented block containing "chrome.runtime.sendMessage({ action:
\"nube-devtools-initialize-sdk\" });" near the end of the focus handler/closing
brace to leave only active code.
In `@packages/nube-devtools/src/hooks/use-nube-status.ts`:
- Around line 3-14: The getTabId helper accesses chrome.tabs.query without
checking chrome or chrome.tabs; update getTabId to first guard that chrome and
chrome.tabs exist (similar to the existing check for
chrome.devtools.inspectedWindow.tabId) before calling chrome.tabs.query, and if
absent call callback(null) (or otherwise return) so the function safely handles
non-extension environments; ensure references to
chrome.devtools.inspectedWindow.tabId, chrome.tabs.query, and the callback
parameter remain intact.
In `@packages/nube-devtools/src/pages/local-mode-content.tsx`:
- Around line 24-25: Extract the duplicated
PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION constant into a single shared constants
module and import it where needed instead of redefining it; update the
declaration in local-mode-content.tsx (the const
PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION) to import the shared constant, and remove
the duplicate definitions from other modules (including the content script) so
all code references use the centralized export (e.g., export const
PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION) to prevent drift.
In `@packages/nube-devtools/src/popup/app.tsx`:
- Around line 6-14: AppContent currently renders Unavailable immediately because
useNubeStatus initializes to false; change the render logic to handle a
loading/unknown state so the Unavailable screen doesn't flash: update AppContent
to treat a third "loading" value (e.g., undefined or null) from useNubeStatus or
add a local loading boolean and render a Loading/Spinner or nothing while status
is pending, then render Unavailable only when status is definitively false and
Popup when true; refer to AppContent, useNubeStatus, Unavailable, and Popup when
making the change.
In `@packages/nube-devtools/src/utils/page-storage.ts`:
- Around line 18-22: Extract the repeated active-tab resolution into a shared
helper (e.g., async function getActiveTabId()) that wraps chrome.tabs.query({
active: true, currentWindow: true }) and returns the tab id or null/false;
replace the three duplicate patterns (the one that queries chrome.tabs in
page-storage.ts at the three locations) with calls to getActiveTabId() and guard
early when it returns no id. Ensure the helper lives alongside other utilities
in the same module and update all call sites to use the helper instead of
duplicating the chrome.tabs.query + if (!tab?.id) return false pattern.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
packages/nube-devtools/package.jsonpackages/nube-devtools/popup.htmlpackages/nube-devtools/public/inject-extension-flag.jspackages/nube-devtools/src/background/index.tspackages/nube-devtools/src/background/nube-dev-tools.tspackages/nube-devtools/src/background/scripts/index.tspackages/nube-devtools/src/background/scripts/inject-window-variable.tspackages/nube-devtools/src/components/copyable-value.tsxpackages/nube-devtools/src/components/footer.tsxpackages/nube-devtools/src/contentScript/index.tspackages/nube-devtools/src/contexts/navigation-context.tsxpackages/nube-devtools/src/devtools/app.tsxpackages/nube-devtools/src/devtools/components/app-detail-panel.tsxpackages/nube-devtools/src/devtools/components/app-sidebar.tsxpackages/nube-devtools/src/devtools/index.csspackages/nube-devtools/src/devtools/pages/LocalMode.tsxpackages/nube-devtools/src/devtools/pages/errors.tsxpackages/nube-devtools/src/devtools/pages/events.tsxpackages/nube-devtools/src/devtools/pages/storages.tsxpackages/nube-devtools/src/devtools/pages/svg-converter.tsxpackages/nube-devtools/src/hooks/use-nube-status.tspackages/nube-devtools/src/manifest.tspackages/nube-devtools/src/pages/local-mode-content.tsxpackages/nube-devtools/src/popup/app.tsxpackages/nube-devtools/src/popup/index.csspackages/nube-devtools/src/popup/index.tsxpackages/nube-devtools/src/popup/popup.tsxpackages/nube-devtools/src/utils/index.tspackages/nube-devtools/src/utils/page-storage.ts
💤 Files with no reviewable changes (4)
- packages/nube-devtools/src/background/scripts/index.ts
- packages/nube-devtools/src/background/nube-dev-tools.ts
- packages/nube-devtools/src/background/scripts/inject-window-variable.ts
- packages/nube-devtools/package.json
lourencorodrigo
force-pushed
the
feat/devtools-server
branch
from
58d2fde to
fd817d5
Compare
lourencorodrigo
force-pushed
the
feat/devtools-server
branch
from
fd817d5 to
f6c6dca
Compare
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a Local Mode UI and popup, introduces page-sessionStorage utilities, replaces executeScript-based window-variable injection with a registered content script flag, updates background messaging and badge logic, and introduces multiple new UI components and styling changes across the devtools package. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/nube-devtools/src/contentScript/index.ts (1)
7-8: Extract the storage key to a shared constant module.This key is duplicated in multiple files; a future rename in one place can silently break sync between UI and content script.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nube-devtools/src/contentScript/index.ts` around lines 7 - 8, The constant PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION is duplicated; extract it into a single shared constants module (e.g., export a named constant like PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION from a new shared/constants or similar module) and replace the local declaration in contentScript/index.ts with an import of that shared constant; also update the other places that currently duplicate the string to import the same exported constant so all code references use the single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/nube-devtools/src/background/index.ts`:
- Around line 20-22: The current blanket catch on the registerContentScripts
call hides all failures; change the catch to inspect the thrown error from
registerContentScripts (e.g., error.message or error.code) and only suppress /
ignore the specific duplicate-registration error (match text like "already
registered" or the platform duplicate-id code), while logging and rethrowing (or
at least not swallowing) all other errors so missing files/permission issues
surface and the flag injection can proceed; update the catch block adjacent to
the registerContentScripts invocation in
packages/nube-devtools/src/background/index.ts to implement this selective
filtering and add a clear error log for non-duplicate failures.
In `@packages/nube-devtools/src/devtools/index.css`:
- Around line 130-134: Add a blank line before the declaration inside the
universal selector to satisfy Stylelint's declaration-empty-line-before rule:
inside the "*" rule block (the selector "*"), insert an empty line before the
"scrollbar-width: thin;" declaration so there's a separation between the "@apply
border-border outline-ring/50;" line and the "scrollbar-width" property.
In `@packages/nube-devtools/src/pages/local-mode-content.tsx`:
- Around line 40-57: The checkBundle function (and the similar check at lines
73-76) can suffer from out-of-order async responses overwriting state; fix by
stamping each check with a unique identifier (e.g., capture the url or
incrementing requestId in a ref) before the fetch, store that identifier as the
"latest" on a ref (like latestCheckUrlRef or latestRequestIdRef), and only call
setStatus / setIsChecking in the try/catch/finally if the identifier still
matches the latest; this ensures stale responses from earlier checkBundle
invocations do not override the current status.
- Around line 161-163: Wrap the JSON.parse(data) call in a try/catch inside the
stop/disconnect flow so a malformed session payload doesn't throw and abort the
disconnect; when parsing fails, log or handle the error and continue with
disconnect logic (e.g., treat as not connected), otherwise proceed to modify
dataObj.connected = false and continue the existing flow — update the block
around JSON.parse(data) and dataObj to use this safe parsing pattern referencing
JSON.parse and dataObj.
- Around line 139-148: The code marks the UI as connected regardless of whether
setPageSessionStorage succeeded; update the flow in the block that calls
setPageSessionStorage (using PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION) to check its
return value and only proceed to setIsConnecting(false) and
setStatus("connected") (and any reload) when setPageSessionStorage returns
truthy; if it returns false, setIsConnecting(false), setStatus("disconnected")
(or an error state/message) and abort the connected flow so the UI does not
display a false connected state.
In `@packages/nube-devtools/src/popup/index.css`:
- Around line 69-84: There are duplicate CSS custom properties for the sidebar
palette (--sidebar, --sidebar-foreground, --sidebar-primary,
--sidebar-primary-foreground, --sidebar-accent, --sidebar-accent-foreground,
--sidebar-border, --sidebar-ring, --sidebar-background) which override the
intended values and trigger Stylelint duplicate-property errors; remove the
repeated declarations so each custom property is defined only once per scope
(keep the intended palette set you want and delete the later conflicting sets),
ensuring the remaining declarations for the identifiers above reflect the
correct color format (oklch vs HSL/percent) and consistent naming so no
duplicate-property lint warnings occur.
---
Nitpick comments:
In `@packages/nube-devtools/src/contentScript/index.ts`:
- Around line 7-8: The constant PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION is
duplicated; extract it into a single shared constants module (e.g., export a
named constant like PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION from a new
shared/constants or similar module) and replace the local declaration in
contentScript/index.ts with an import of that shared constant; also update the
other places that currently duplicate the string to import the same exported
constant so all code references use the single source of truth.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
packages/nube-devtools/package.jsonpackages/nube-devtools/popup.htmlpackages/nube-devtools/public/inject-extension-flag.jspackages/nube-devtools/src/background/index.tspackages/nube-devtools/src/background/nube-dev-tools.tspackages/nube-devtools/src/background/scripts/index.tspackages/nube-devtools/src/background/scripts/inject-window-variable.tspackages/nube-devtools/src/components/copyable-value.tsxpackages/nube-devtools/src/components/footer.tsxpackages/nube-devtools/src/contentScript/index.tspackages/nube-devtools/src/contexts/navigation-context.tsxpackages/nube-devtools/src/devtools/app.tsxpackages/nube-devtools/src/devtools/components/app-detail-panel.tsxpackages/nube-devtools/src/devtools/components/app-sidebar.tsxpackages/nube-devtools/src/devtools/index.csspackages/nube-devtools/src/devtools/pages/LocalMode.tsxpackages/nube-devtools/src/devtools/pages/errors.tsxpackages/nube-devtools/src/devtools/pages/events.tsxpackages/nube-devtools/src/devtools/pages/storages.tsxpackages/nube-devtools/src/devtools/pages/svg-converter.tsxpackages/nube-devtools/src/hooks/use-nube-status.tspackages/nube-devtools/src/manifest.tspackages/nube-devtools/src/pages/local-mode-content.tsxpackages/nube-devtools/src/popup/app.tsxpackages/nube-devtools/src/popup/index.csspackages/nube-devtools/src/popup/index.tsxpackages/nube-devtools/src/popup/popup.tsxpackages/nube-devtools/src/utils/index.tspackages/nube-devtools/src/utils/page-storage.ts
💤 Files with no reviewable changes (4)
- packages/nube-devtools/src/background/scripts/index.ts
- packages/nube-devtools/package.json
- packages/nube-devtools/src/background/nube-dev-tools.ts
- packages/nube-devtools/src/background/scripts/inject-window-variable.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/nube-devtools/src/popup/popup.tsx
- packages/nube-devtools/src/components/copyable-value.tsx
- packages/nube-devtools/src/hooks/use-nube-status.ts
- packages/nube-devtools/src/popup/index.tsx
- packages/nube-devtools/src/manifest.ts
- packages/nube-devtools/src/contexts/navigation-context.tsx
- packages/nube-devtools/src/devtools/pages/svg-converter.tsx
- packages/nube-devtools/src/utils/page-storage.ts
- packages/nube-devtools/src/devtools/pages/events.tsx
- packages/nube-devtools/popup.html
- packages/nube-devtools/public/inject-extension-flag.js
- packages/nube-devtools/src/components/footer.tsx
lourencorodrigo
changed the title
lourencorodrigo
force-pushed
the
feat/devtools-server
branch
from
f6c6dca to
6623dc1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
packages/nube-devtools/src/devtools/pages/svg-converter.tsx (1)
154-154: Remove the emptyclassNameprop fromSyntaxHighlighter.Line 154 contains
className="", which is a no-op and non-idiomatic. Omit the prop entirely unless applying an actual CSS class.Suggested diff
- className=""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nube-devtools/src/devtools/pages/svg-converter.tsx` at line 154, Remove the no-op empty className attribute from the SyntaxHighlighter usage in svg-converter.tsx: locate the JSX element <SyntaxHighlighter ... className=""> and delete the className="" prop entirely (leave other props intact) so the component is not passed an empty string class.packages/nube-devtools/src/components/ui/native-select.tsx (1)
10-29:classNamecurrently styles the wrapper, not the<select>control.This makes consumer styling for the actual input element harder/unpredictable. Consider splitting wrapper/select class props.
♻️ Suggested refactor
type NativeSelectProps = Omit<React.ComponentProps<"select">, "size"> & { size?: "sm" | "default"; + wrapperClassName?: string; }; function NativeSelect({ className, + wrapperClassName, size = "default", ...props }: NativeSelectProps) { return ( <div className={cn( "group/native-select relative w-fit has-[select:disabled]:opacity-50", - className, + wrapperClassName, )} data-slot="native-select-wrapper" data-size={size} > <select data-slot="native-select" data-size={size} - className="border-input placeholder:text-muted-foreground selection:bg-primary selection:text-primary-foreground dark:bg-input/30 dark:hover:bg-input/50 focus-visible:border-ring focus-visible:ring-ring/50 aria-invalid:ring-destructive/20 dark:aria-invalid:ring-destructive/40 aria-invalid:border-destructive dark:aria-invalid:border-destructive/50 h-8 w-full min-w-0 appearance-none rounded-lg border bg-transparent py-1 pr-8 pl-2.5 text-sm transition-colors select-none focus-visible:ring-3 aria-invalid:ring-3 data-[size=sm]:h-7 data-[size=sm]:rounded-[min(var(--radius-md),10px)] data-[size=sm]:py-0.5 outline-none disabled:pointer-events-none disabled:cursor-not-allowed" + className={cn( + "border-input placeholder:text-muted-foreground selection:bg-primary selection:text-primary-foreground dark:bg-input/30 dark:hover:bg-input/50 focus-visible:border-ring focus-visible:ring-ring/50 aria-invalid:ring-destructive/20 dark:aria-invalid:ring-destructive/40 aria-invalid:border-destructive dark:aria-invalid:border-destructive/50 h-8 w-full min-w-0 appearance-none rounded-lg border bg-transparent py-1 pr-8 pl-2.5 text-sm transition-colors select-none focus-visible:ring-3 aria-invalid:ring-3 data-[size=sm]:h-7 data-[size=sm]:rounded-[min(var(--radius-md),10px)] data-[size=sm]:py-0.5 outline-none disabled:pointer-events-none disabled:cursor-not-allowed", + className, + )} {...props} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nube-devtools/src/components/ui/native-select.tsx` around lines 10 - 29, The current NativeSelect component applies the incoming className to the wrapper div instead of the <select>, which prevents consumers from styling the actual control; update the component API and implementation so className targets the <select> (or add a new prop like wrapperClassName for the outer div and keep className for the select). Concretely, change NativeSelectProps to include wrapperClassName?: string and className?: string (or swap semantics so className is applied to the select), then move the cn(...) usage: apply wrapperClassName to the div's className and apply the composed select classes plus className to the <select> element (ensure data-size handling and existing default select classes in NativeSelect are preserved).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/nube-devtools/src/components/copyable-value.tsx`:
- Around line 9-13: The handleCopy function (navigator.clipboard.writeText call
inside handleCopy) lacks error handling and can produce unhandled Promise
rejections; update handleCopy to handle both success and failure by adding a
.catch (or try/catch if converting to async) to call toast.error with a helpful
message on failure and optionally implement a fallback copy approach (e.g.,
legacy document.execCommand or creating a temporary textarea) for environments
where navigator.clipboard is unavailable; ensure the change references the
handleCopy callback in copyable-value.tsx so the Promise is always handled.
In `@packages/nube-devtools/src/components/local-mode-content.tsx`:
- Around line 300-325: markStoredAsDisconnected can throw on JSON.parse which
will abort handleStop before UI/state cleanup; wrap the read/parse/update
sequence in markStoredAsDisconnected with a try/catch that handles malformed
JSON (e.g., treat as absent or overwrite with a safe disconnected payload) and
never rethrow so callers like handleStop always continue. Update
markStoredAsDisconnected to catch parsing errors, optionally remove or overwrite
the PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION entry with a known disconnected
object, and ensure await setPageSessionStorage is still called when appropriate;
leave handleStop unchanged so its subsequent state resets always run even if
stored data was malformed.
- Around line 229-235: reloadCurrentTab currently reloads the active tab via
chrome.tabs.query which can target the wrong tab in DevTools; change
reloadCurrentTab to first check for chrome.devtools?.inspectedWindow?.tabId and
call chrome.tabs.reload with that id if present, otherwise fall back to the
existing chrome.tabs.query active/currentWindow logic and reload the found tab;
update references in reloadCurrentTab (chrome.devtools.inspectedWindow.tabId,
chrome.tabs.query, chrome.tabs.reload) accordingly to preserve behavior when
devtools isn't available.
In `@packages/nube-devtools/src/devtools/index.css`:
- Around line 130-137: Stylelint flagged a missing empty line before the
scrollbar-width declaration in the global rule; open the rule for the universal
selector (*) where you set `@apply` border-border outline-ring/50 and insert a
single blank line immediately before the scrollbar-width: thin; declaration (the
declaration within the "*" rule that precedes scrollbar-color), then save/format
so the same spacing is applied consistently for the scrollbar-color lines
(including the .dark * block) to satisfy Stylelint.
In `@packages/nube-devtools/src/popup/index.css`:
- Around line 69-84: Duplicate CSS custom properties for the sidebar are defined
twice (the oklch() set and a later HSL-like numeric set) causing silent
overrides and Stylelint failures; remove the redundant redefinitions so each
--sidebar custom property is declared only once (keep either the oklch(...)
definitions or the numeric HSL-like definitions consistently), e.g., remove the
second group of --sidebar, --sidebar-foreground, --sidebar-primary,
--sidebar-primary-foreground, --sidebar-accent, --sidebar-accent-foreground,
--sidebar-border, and --sidebar-ring declarations in
packages/nube-devtools/src/popup/index.css so each variable is uniquely defined.
- Around line 4-6: Stylelint is flagging Tailwind v4 directives (`@custom-variant`
and `@theme`) as unknown; update the Stylelint config (.stylelintrc.json) to allow
these at-rules by adding an ignoreAtRules entry to the at-rule-no-unknown
rule—specifically modify the at-rule-no-unknown configuration to include
"custom-variant", "theme" (and "tailwind" if desired) in ignoreAtRules so the
directives used in index.css are accepted.
---
Nitpick comments:
In `@packages/nube-devtools/src/components/ui/native-select.tsx`:
- Around line 10-29: The current NativeSelect component applies the incoming
className to the wrapper div instead of the <select>, which prevents consumers
from styling the actual control; update the component API and implementation so
className targets the <select> (or add a new prop like wrapperClassName for the
outer div and keep className for the select). Concretely, change
NativeSelectProps to include wrapperClassName?: string and className?: string
(or swap semantics so className is applied to the select), then move the cn(...)
usage: apply wrapperClassName to the div's className and apply the composed
select classes plus className to the <select> element (ensure data-size handling
and existing default select classes in NativeSelect are preserved).
In `@packages/nube-devtools/src/devtools/pages/svg-converter.tsx`:
- Line 154: Remove the no-op empty className attribute from the
SyntaxHighlighter usage in svg-converter.tsx: locate the JSX element
<SyntaxHighlighter ... className=""> and delete the className="" prop entirely
(leave other props intact) so the component is not passed an empty string class.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
packages/nube-devtools/package.jsonpackages/nube-devtools/popup.htmlpackages/nube-devtools/public/inject-extension-flag.jspackages/nube-devtools/src/background/index.tspackages/nube-devtools/src/background/nube-dev-tools.tspackages/nube-devtools/src/background/scripts/index.tspackages/nube-devtools/src/background/scripts/inject-window-variable.tspackages/nube-devtools/src/components/copyable-value.tsxpackages/nube-devtools/src/components/local-mode-card-header.tsxpackages/nube-devtools/src/components/local-mode-content.tsxpackages/nube-devtools/src/components/local-mode-quick-tips.tsxpackages/nube-devtools/src/components/local-mode-status-messages.tsxpackages/nube-devtools/src/components/ui/native-select.tsxpackages/nube-devtools/src/contentScript/index.tspackages/nube-devtools/src/contexts/navigation-context.tsxpackages/nube-devtools/src/devtools/app.tsxpackages/nube-devtools/src/devtools/components/app-detail-panel.tsxpackages/nube-devtools/src/devtools/components/app-sidebar.tsxpackages/nube-devtools/src/devtools/index.csspackages/nube-devtools/src/devtools/pages/LocalMode.tsxpackages/nube-devtools/src/devtools/pages/apps.tsxpackages/nube-devtools/src/devtools/pages/errors.tsxpackages/nube-devtools/src/devtools/pages/events.tsxpackages/nube-devtools/src/devtools/pages/storages.tsxpackages/nube-devtools/src/devtools/pages/svg-converter.tsxpackages/nube-devtools/src/hooks/use-nube-status.tspackages/nube-devtools/src/manifest.tspackages/nube-devtools/src/popup/app.tsxpackages/nube-devtools/src/popup/components/popup-footer.tsxpackages/nube-devtools/src/popup/index.csspackages/nube-devtools/src/popup/index.tsxpackages/nube-devtools/src/popup/popup.tsxpackages/nube-devtools/src/utils/index.tspackages/nube-devtools/src/utils/page-storage.ts
💤 Files with no reviewable changes (3)
- packages/nube-devtools/src/background/scripts/inject-window-variable.ts
- packages/nube-devtools/src/background/nube-dev-tools.ts
- packages/nube-devtools/src/background/scripts/index.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/nube-devtools/src/contexts/navigation-context.tsx
- packages/nube-devtools/src/popup/app.tsx
- packages/nube-devtools/src/utils/page-storage.ts
- packages/nube-devtools/src/devtools/pages/errors.tsx
- packages/nube-devtools/src/hooks/use-nube-status.ts
- packages/nube-devtools/src/popup/popup.tsx
- packages/nube-devtools/package.json
- packages/nube-devtools/src/contentScript/index.ts
- packages/nube-devtools/src/devtools/pages/LocalMode.tsx
- packages/nube-devtools/src/manifest.ts
- packages/nube-devtools/src/popup/index.tsx
- packages/nube-devtools/src/devtools/pages/storages.tsx
- packages/nube-devtools/popup.html
- packages/nube-devtools/src/utils/index.ts
- packages/nube-devtools/src/devtools/components/app-detail-panel.tsx
- packages/nube-devtools/src/devtools/app.tsx
- packages/nube-devtools/src/background/index.ts
lourencorodrigo
force-pushed
the
feat/devtools-server
branch
from
6623dc1 to
e7960d4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
packages/nube-devtools/src/devtools/index.css (1)
131-133:⚠️ Potential issue | 🟡 MinorFix Stylelint spacing before
scrollbar-width.At Line 132, add a blank line between
@applyandscrollbar-widthto satisfydeclaration-empty-line-before.🧹 Proposed fix
* { `@apply` border-border outline-ring/50; + scrollbar-width: thin; scrollbar-color: oklch(0.7 0 0 / 30%) transparent; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nube-devtools/src/devtools/index.css` around lines 131 - 133, Add a single blank line before the declaration "scrollbar-width: thin;" so there is an empty line between the "@apply border-border outline-ring/50;" rule and the "scrollbar-width" declaration to satisfy the declaration-empty-line-before Stylelint rule; update the CSS block containing "@apply border-border outline-ring/50;", "scrollbar-width: thin;", and "scrollbar-color: oklch(0.7 0 0 / 30%) transparent;" by inserting one empty line between the `@apply` line and the scrollbar-width line.packages/nube-devtools/src/components/copyable-value.tsx (1)
9-13:⚠️ Potential issue | 🟡 MinorHandle clipboard write failures to prevent unhandled Promise rejections.
The
navigator.clipboard.writeText()call lacks error handling. If the operation fails (e.g., due to permissions or unavailable context), the Promise rejection goes unhandled and the user receives no feedback.🛡️ Proposed fix
- const handleCopy = useCallback(() => { - navigator.clipboard.writeText(value).then(() => { - toast.success("Copied to clipboard"); - }); - }, [value]); + const handleCopy = useCallback(async () => { + try { + await navigator.clipboard.writeText(value); + toast.success("Copied to clipboard"); + } catch { + toast.error("Unable to copy to clipboard"); + } + }, [value]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nube-devtools/src/components/copyable-value.tsx` around lines 9 - 13, The handleCopy callback currently calls navigator.clipboard.writeText(value) without handling rejections; update handleCopy (the useCallback function) to catch errors from navigator.clipboard.writeText(value) and provide user feedback and logging on failure (e.g., call toast.error with a helpful message and optionally console.error the error), ensuring both the success path still calls toast.success and the failure path prevents unhandled Promise rejections.packages/nube-devtools/src/components/local-mode-content.tsx (2)
290-304:⚠️ Potential issue | 🟠 MajorMalformed stored JSON can break Stop flow before state cleanup runs.
JSON.parseat line 296 can throw on malformed data, preventinghandleStopfrom completing its state reset. TheloadStoredServerfunction (lines 215-220) correctly wraps itsJSON.parsein try-catch, but this function doesn't.🛡️ Suggested hardening
const markStoredAsDisconnected = async () => { const stored = await getPageSessionStorage( PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION, ); if (!stored) return; - const data = JSON.parse(stored) as StoredApplicationServerData; - if (!data.connected) return; - - data.connected = false; - await setPageSessionStorage( - PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION, - JSON.stringify(data), - ); + try { + const data = JSON.parse(stored) as StoredApplicationServerData; + if (!data.connected) return; + data.connected = false; + await setPageSessionStorage( + PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION, + JSON.stringify(data), + ); + } catch { + // Ignore malformed payload; caller cleanup continues. + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nube-devtools/src/components/local-mode-content.tsx` around lines 290 - 304, The markStoredAsDisconnected handler can throw on malformed stored JSON because it calls JSON.parse directly; wrap the parse in a try-catch inside markStoredAsDisconnected (the function using getPageSessionStorage, JSON.parse, and setPageSessionStorage) so parse failures do not abort the stop flow—on error, treat it as already-disconnected (or remove/clear PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION) and continue, ensuring data.connected is only updated when parse succeeds and the function always returns without throwing.
225-231:⚠️ Potential issue | 🟠 MajorReload should prioritize the inspected tab, not only the active tab query.
In DevTools workflows, the active tab may differ from the inspected tab. Use
chrome.devtools.inspectedWindow.tabIdas the primary target, consistent with the pattern used infetchApps(lines 108-119).🔧 Suggested fix
const reloadCurrentTab = async () => { + const inspectedTabId = chrome.devtools?.inspectedWindow?.tabId; + if (inspectedTabId != null) { + chrome.tabs.reload(inspectedTabId); + return; + } const [tab] = await chrome.tabs.query({ active: true, currentWindow: true, }); - if (tab?.id) chrome.tabs.reload(tab.id); + if (tab?.id != null) chrome.tabs.reload(tab.id); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nube-devtools/src/components/local-mode-content.tsx` around lines 225 - 231, The reloadCurrentTab function currently queries the active tab but should prioritize the DevTools inspected tab; update reloadCurrentTab to first read chrome.devtools.inspectedWindow.tabId and, if it's a number, call chrome.tabs.reload with that id, otherwise fall back to the existing chrome.tabs.query active/currentWindow lookup and reload the resulting tab id; keep the existing null/undefined checks (tab?.id) and ensure usage mirrors the pattern in fetchApps by treating inspectedWindow.tabId as the primary target.
🧹 Nitpick comments (4)
packages/nube-devtools/src/components/local-mode-content.tsx (2)
270-281: Connection data persistsconnected: truebefore script verification completes.The storage is set to
connected: true(line 272) beforecheckScriptAvailabilityruns. If the script is unavailable, the stored state indicates "connected" while the UI shows "unavailable". This inconsistency could cause confusion on page reload. Consider updating the stored state after verification completes, or clarifying that "connected" means "connection initiated" rather than "script verified".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nube-devtools/src/components/local-mode-content.tsx` around lines 270 - 281, The code is persisting connected: true before script verification completes; update the flow so persistConnectionData(...) is called only after checkScriptAvailability(...) confirms availability (or persist with connected: false initially and then update it after verification). Locate the calls to persistConnectionData, checkScriptAvailability, setIsConnecting, setStatus, and notifyConnectionStatus (and the variables trimmedUrl, resolvedAppId, selectedTab) and either move the successful-persist call to after the verification step or perform an initial persist with connected: false and then a second persist/update to connected: true when verification succeeds; ensure notifyConnectionStatus and setStatus reflect the verified state.
63-67: Consider validating localStorage value forselectedTab.If localStorage contains an unexpected value (e.g., corrupted or manually edited), the type cast assumes it's valid. A safer approach would validate the value.
🛡️ Suggested improvement
- const [selectedTab, setSelectedTab] = useState<"new" | "existing">( - (localStorage.getItem(LOCAL_STORAGE_KEY_DEVTOOLS_LOCAL_MODE_SELECTED_TAB) as - | "new" - | "existing") || "new", - ); + const [selectedTab, setSelectedTab] = useState<"new" | "existing">(() => { + const stored = localStorage.getItem(LOCAL_STORAGE_KEY_DEVTOOLS_LOCAL_MODE_SELECTED_TAB); + return stored === "new" || stored === "existing" ? stored : "new"; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nube-devtools/src/components/local-mode-content.tsx` around lines 63 - 67, The current initialization of selectedTab in local-mode-content.tsx blindly casts the localStorage value to "new" | "existing", which can break if the stored value is unexpected; update the initialization (or add a small helper like getInitialSelectedTab) to read LOCAL_STORAGE_KEY_DEVTOOLS_LOCAL_MODE_SELECTED_TAB, check whether the value strictly equals "new" or "existing", and only then return it, otherwise return the safe default "new" and ensure setSelectedTab continues to persist only valid values.packages/nube-devtools/src/components/local-mode-status-messages.tsx (1)
43-50: Consider adding dark mode text styling for consistency.The "available" state (line 38) has
dark:text-green-400for dark mode support, but the "unavailable" state only usestext-destructive. If the destructive color isn't already dark-mode aware via CSS variables, this could look inconsistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nube-devtools/src/components/local-mode-status-messages.tsx` around lines 43 - 50, The "unavailable" branch lacks a dark-mode text variant; update the element(s) that use text-destructive (the <p> with class "text-destructive" and optionally the AlertCircle icon with "text-destructive") to include a dark-mode class (e.g., dark:text-destructive or a specific dark variant like dark:text-destructive-400) so the destructive text/icon color matches dark-mode styling similar to the "available" branch which uses dark:text-green-400; modify the paragraph and icon class lists in the component where scriptAvailability === "unavailable" to add the appropriate dark: class names.packages/nube-devtools/src/contentScript/index.ts (1)
34-36: Remove commented-out initialization message block.Please drop the dead commented code (or gate it with a real feature flag) to avoid stale behavior hints in runtime code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nube-devtools/src/contentScript/index.ts` around lines 34 - 36, Remove the dead commented initialization message block in packages/nube-devtools/src/contentScript/index.ts: delete the commented chrome.runtime.sendMessage({ action: "nube-devtools-initialize-sdk" }) lines (or, if this behavior is still required, reintroduce it behind a real feature flag check such as isSdkInitEnabled and call chrome.runtime.sendMessage from the initialization flow). Ensure you update any related comments to reflect the change and keep the contentScript initialization logic (index.ts) free of stale commented-out runtime code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/nube-devtools/src/components/local-mode-content.tsx`:
- Around line 354-362: The two Input elements both use id="app-server-url",
violating unique-ID rules; change them to distinct IDs (e.g.,
"app-server-url-new" and "app-server-url-existing") and update their matching
<label> htmlFor attributes so labels point to the correct input; also search for
any JS that references "app-server-url" (e.g., getElementById or tests) and
update those references to the new IDs, leaving handlers like handleUrlChange,
and state variables appUrl/isConnecting/status unchanged.
In `@packages/nube-devtools/src/hooks/use-nube-status.ts`:
- Around line 27-30: The hook in use-nube-status ignores falsy boolean responses
because it only updates state when response.status is truthy; change the update
logic in the response handler used by setNubeStatus so it sets nubeStatus to
response.status whenever response has a status property (including false) —
e.g., check for response !== undefined or 'status' in response and call
setNubeStatus(() => response.status) so false values propagate properly.
- Around line 3-13: getTabId currently calls chrome.tabs.query unguarded which
can throw when chrome or chrome.tabs is undefined and also ignores
chrome.runtime.lastError; update getTabId to first check for typeof chrome !==
"undefined" and chrome.tabs before calling chrome.tabs.query, and if chrome or
chrome.tabs is missing call callback(null) immediately; when using
chrome.tabs.query, inspect chrome.runtime.lastError in its callback and if an
error exists pass null (or handle appropriately) to the provided callback,
otherwise pass tabs[0]?.id; reference getTabId,
chrome.devtools?.inspectedWindow?.tabId, chrome.tabs.query and
chrome.runtime.lastError when making the change.
In `@packages/nube-devtools/src/utils/page-storage.ts`:
- Around line 18-22: Wrap all chrome.tabs.query calls in a try/catch and extract
a new helper named getActiveTabId() that returns the active tab id or null/false
on failure; replace the inline queries (the one currently assigning const [tab]
= await chrome.tabs.query({ active: true, currentWindow: true }); plus the
similar occurrences around the other two blocks) with calls to getActiveTabId(),
and ensure getActiveTabId() handles exceptions from chrome.tabs.query() and
returns the documented fallback (false or null) so the surrounding functions
preserve their contract.
---
Duplicate comments:
In `@packages/nube-devtools/src/components/copyable-value.tsx`:
- Around line 9-13: The handleCopy callback currently calls
navigator.clipboard.writeText(value) without handling rejections; update
handleCopy (the useCallback function) to catch errors from
navigator.clipboard.writeText(value) and provide user feedback and logging on
failure (e.g., call toast.error with a helpful message and optionally
console.error the error), ensuring both the success path still calls
toast.success and the failure path prevents unhandled Promise rejections.
In `@packages/nube-devtools/src/components/local-mode-content.tsx`:
- Around line 290-304: The markStoredAsDisconnected handler can throw on
malformed stored JSON because it calls JSON.parse directly; wrap the parse in a
try-catch inside markStoredAsDisconnected (the function using
getPageSessionStorage, JSON.parse, and setPageSessionStorage) so parse failures
do not abort the stop flow—on error, treat it as already-disconnected (or
remove/clear PAGE_STORAGE_KEY_DEVTOOLS_APPLICATION) and continue, ensuring
data.connected is only updated when parse succeeds and the function always
returns without throwing.
- Around line 225-231: The reloadCurrentTab function currently queries the
active tab but should prioritize the DevTools inspected tab; update
reloadCurrentTab to first read chrome.devtools.inspectedWindow.tabId and, if
it's a number, call chrome.tabs.reload with that id, otherwise fall back to the
existing chrome.tabs.query active/currentWindow lookup and reload the resulting
tab id; keep the existing null/undefined checks (tab?.id) and ensure usage
mirrors the pattern in fetchApps by treating inspectedWindow.tabId as the
primary target.
In `@packages/nube-devtools/src/devtools/index.css`:
- Around line 131-133: Add a single blank line before the declaration
"scrollbar-width: thin;" so there is an empty line between the "@apply
border-border outline-ring/50;" rule and the "scrollbar-width" declaration to
satisfy the declaration-empty-line-before Stylelint rule; update the CSS block
containing "@apply border-border outline-ring/50;", "scrollbar-width: thin;",
and "scrollbar-color: oklch(0.7 0 0 / 30%) transparent;" by inserting one empty
line between the `@apply` line and the scrollbar-width line.
---
Nitpick comments:
In `@packages/nube-devtools/src/components/local-mode-content.tsx`:
- Around line 270-281: The code is persisting connected: true before script
verification completes; update the flow so persistConnectionData(...) is called
only after checkScriptAvailability(...) confirms availability (or persist with
connected: false initially and then update it after verification). Locate the
calls to persistConnectionData, checkScriptAvailability, setIsConnecting,
setStatus, and notifyConnectionStatus (and the variables trimmedUrl,
resolvedAppId, selectedTab) and either move the successful-persist call to after
the verification step or perform an initial persist with connected: false and
then a second persist/update to connected: true when verification succeeds;
ensure notifyConnectionStatus and setStatus reflect the verified state.
- Around line 63-67: The current initialization of selectedTab in
local-mode-content.tsx blindly casts the localStorage value to "new" |
"existing", which can break if the stored value is unexpected; update the
initialization (or add a small helper like getInitialSelectedTab) to read
LOCAL_STORAGE_KEY_DEVTOOLS_LOCAL_MODE_SELECTED_TAB, check whether the value
strictly equals "new" or "existing", and only then return it, otherwise return
the safe default "new" and ensure setSelectedTab continues to persist only valid
values.
In `@packages/nube-devtools/src/components/local-mode-status-messages.tsx`:
- Around line 43-50: The "unavailable" branch lacks a dark-mode text variant;
update the element(s) that use text-destructive (the <p> with class
"text-destructive" and optionally the AlertCircle icon with "text-destructive")
to include a dark-mode class (e.g., dark:text-destructive or a specific dark
variant like dark:text-destructive-400) so the destructive text/icon color
matches dark-mode styling similar to the "available" branch which uses
dark:text-green-400; modify the paragraph and icon class lists in the component
where scriptAvailability === "unavailable" to add the appropriate dark: class
names.
In `@packages/nube-devtools/src/contentScript/index.ts`:
- Around line 34-36: Remove the dead commented initialization message block in
packages/nube-devtools/src/contentScript/index.ts: delete the commented
chrome.runtime.sendMessage({ action: "nube-devtools-initialize-sdk" }) lines
(or, if this behavior is still required, reintroduce it behind a real feature
flag check such as isSdkInitEnabled and call chrome.runtime.sendMessage from the
initialization flow). Ensure you update any related comments to reflect the
change and keep the contentScript initialization logic (index.ts) free of stale
commented-out runtime code.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
packages/nube-devtools/package.jsonpackages/nube-devtools/popup.htmlpackages/nube-devtools/public/inject-extension-flag.jspackages/nube-devtools/src/background/index.tspackages/nube-devtools/src/background/nube-dev-tools.tspackages/nube-devtools/src/background/scripts/index.tspackages/nube-devtools/src/background/scripts/inject-window-variable.tspackages/nube-devtools/src/components/copyable-value.tsxpackages/nube-devtools/src/components/local-mode-card-header.tsxpackages/nube-devtools/src/components/local-mode-content.tsxpackages/nube-devtools/src/components/local-mode-quick-tips.tsxpackages/nube-devtools/src/components/local-mode-status-messages.tsxpackages/nube-devtools/src/components/ui/native-select.tsxpackages/nube-devtools/src/contentScript/index.tspackages/nube-devtools/src/contexts/navigation-context.tsxpackages/nube-devtools/src/devtools/app.tsxpackages/nube-devtools/src/devtools/components/app-detail-panel.tsxpackages/nube-devtools/src/devtools/components/app-sidebar.tsxpackages/nube-devtools/src/devtools/index.csspackages/nube-devtools/src/devtools/pages/LocalMode.tsxpackages/nube-devtools/src/devtools/pages/apps.tsxpackages/nube-devtools/src/devtools/pages/errors.tsxpackages/nube-devtools/src/devtools/pages/events.tsxpackages/nube-devtools/src/devtools/pages/storages.tsxpackages/nube-devtools/src/devtools/pages/svg-converter.tsxpackages/nube-devtools/src/hooks/use-nube-status.tspackages/nube-devtools/src/manifest.tspackages/nube-devtools/src/popup/app.tsxpackages/nube-devtools/src/popup/components/popup-footer.tsxpackages/nube-devtools/src/popup/index.csspackages/nube-devtools/src/popup/index.tsxpackages/nube-devtools/src/popup/popup.tsxpackages/nube-devtools/src/utils/index.tspackages/nube-devtools/src/utils/page-storage.ts
💤 Files with no reviewable changes (3)
- packages/nube-devtools/src/background/nube-dev-tools.ts
- packages/nube-devtools/src/background/scripts/inject-window-variable.ts
- packages/nube-devtools/src/background/scripts/index.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- packages/nube-devtools/src/devtools/pages/storages.tsx
- packages/nube-devtools/src/popup/components/popup-footer.tsx
- packages/nube-devtools/src/popup/popup.tsx
- packages/nube-devtools/package.json
- packages/nube-devtools/src/components/ui/native-select.tsx
- packages/nube-devtools/src/popup/app.tsx
- packages/nube-devtools/src/components/local-mode-quick-tips.tsx
- packages/nube-devtools/src/devtools/components/app-detail-panel.tsx
- packages/nube-devtools/src/devtools/pages/apps.tsx
- packages/nube-devtools/src/popup/index.tsx
- packages/nube-devtools/src/devtools/components/app-sidebar.tsx
- packages/nube-devtools/popup.html
- packages/nube-devtools/src/components/local-mode-card-header.tsx
- packages/nube-devtools/src/devtools/pages/errors.tsx
- packages/nube-devtools/src/devtools/pages/svg-converter.tsx
- packages/nube-devtools/src/background/index.ts
- packages/nube-devtools/src/devtools/pages/LocalMode.tsx
- packages/nube-devtools/public/inject-extension-flag.js
- packages/nube-devtools/src/manifest.ts
Comment on lines
27
to
30
| (response) => { | ||
| if (response?.status) { | ||
| setNubeStatus(() => response.status); | ||
| } |
Contributor
There was a problem hiding this comment.
Propagate false status values instead of treating them as no-op.
Line 28 only updates for truthy values; when background returns status: false, the hook does not clear nubeStatus.
Proposed fix
(response) => {
- if (response?.status) {
- setNubeStatus(() => response.status);
+ if (typeof response?.status === "boolean") {
+ setNubeStatus(response.status);
}
},📝 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.
Suggested change
| (response) => { | |
| if (response?.status) { | |
| setNubeStatus(() => response.status); | |
| } | |
| (response) => { | |
| if (typeof response?.status === "boolean") { | |
| setNubeStatus(response.status); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/nube-devtools/src/hooks/use-nube-status.ts` around lines 27 - 30,
The hook in use-nube-status ignores falsy boolean responses because it only
updates state when response.status is truthy; change the update logic in the
response handler used by setNubeStatus so it sets nubeStatus to response.status
whenever response has a status property (including false) — e.g., check for
response !== undefined or 'status' in response and call setNubeStatus(() =>
response.status) so false values propagate properly.
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.
Summary
page-storageutilities to read/write the active tab'ssessionStoragefrom the extension contextpopup.html), footer component, and inject-extension-flag content scriptScreenshots
Summary by CodeRabbit
New Features
Style
Chores