feat(migration-helper): v0.3.57 — in-place token substitution + tooltip fix - #1349
feat(migration-helper): v0.3.57 — in-place token substitution + tooltip fix#1349Belu Montoya (belumontoya) wants to merge 3 commits into
Conversation
…p fix Replace mode reworked: each dialtone-css-* tag now has its token values substituted in-place (structure, mode blocks, @layer wrappers, names preserved). No tag is blanked, no framework CSS injected. Reverse-rename tables (spacing←space, layout←size, positive←success) added in token-renames.js so mechanically-renamed families also resolve to stable values. Token tooltip utility-class section now shows declarations one per line (custom props only, capped at 6) instead of a raw cssText blob. eslint.config.js: browser + chrome extension globals override for the migration helper package.
|
Adds the Dialtone migration-helper Chrome MV3 extension to the monorepo, fixing stable-token “replace” via byte-identical in-place substitution for Overall Judgement: ✅ Ready to merge — the changes are cohesive, reversible, and backed by expanded automated E2E verification. WalkthroughAdds a Chrome MV3 extension package for Dialtone migration helpers, including background token/CSS caching, content-script modes for inspect/override/avatar swap/replace, popup controls, analysis data, capture/diff scripts, and Playwright e2e coverage. ChangesDialtone Migration Helper Extension
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b64a6b9d25
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@eslint.config.js`:
- Around line 49-64: The Chrome extension override is disabling no-undef too
broadly, which removes typo and ReferenceError protection. In the
eslint.config.js override for packages/dialtone-migration-helper, keep
globals.browser and chrome: 'readonly' but re-enable no-undef so genuine
undefined-variable mistakes in background.js/content.js are still caught, while
preserving the other relaxed rules in that scoped config block.
In `@packages/dialtone-migration-helper/background.js`:
- Around line 74-76: The caching logic in the helper currently reuses a stable
cache entry for `latest`, which can leave `background.js` comparing against an
outdated resolved version. Update the `cacheKey`/lookup flow used in `getCached`
(and the related cache reads at the other referenced call sites) so `latest` is
either not cached at all or is revalidated with a short TTL or resolved-version
check before returning `cached`. Keep the change localized around the existing
`cacheKey` and `chrome.storage.local.get` usage.
In `@packages/dialtone-migration-helper/content.js`:
- Around line 1-9: Update the header comment in content.js so it matches the
current toggle/state model: it should describe the four toggles managed by the
extension, not a single dtInspect flag, and note that state is persisted per-tab
through the background worker using chrome.storage.session rather than
chrome.storage.local. Reference the current toggle/state flow by name
(dtInspect, dtOverride, dtAvatarSwap, dtReplace, and the background.js
getTabState/setTabState helpers) so the documentation stays aligned with the
implementation.
- Around line 508-546: The selector in captureOverrideSnapshot is too broad and
causes unnecessary layout work before the 80-item cap is applied. Update the
class filter used in captureOverrideSnapshot to match only real Dialtone utility
classes, consistent with collectUtilitiesForElement’s startsWith('d-') logic,
and ensure isVisibleElement/getBoundingClientRect runs only after narrowing the
candidate set as much as possible. This should keep the snapshot accurate and
avoid scanning unrelated classes like card-body or id-1.
- Around line 1026-1043: The loadStableMap() flow assigns to stableTheme even
though no such variable is declared, which will throw in strict mode. Declare
stableTheme alongside stableMap near the top-level state in content.js, and
ensure loadStableMap() continues to set it after a successful
sendMessage/getStableTokens response. Keep the fix localized to the
loadStableMap() path and the shared state declarations used by activateInspect
and the storage.onChanged listener.
In `@packages/dialtone-migration-helper/e2e.mjs`:
- Around line 9-10: The e2e.mjs script currently hardcodes an internal
Firespotter beta URL in DEFAULT_FIRESPOTTER_URL, which would leak private host
and identifiers in the published package and failure output. Replace that
constant with a neutral placeholder or require REAL_PAGE_URL to be provided at
runtime, and update the related fallback/error handling in the affected flows so
they no longer reference the internal URL. Use DEFAULT_FIRESPOTTER_URL and any
code paths that consume it to locate and remove the hardcoded value.
- Around line 163-166: The token-renames.js load in e2e.mjs is being swallowed,
which hides missing file, syntax, or script injection failures and leads to
misleading TS-009 assertions. Update the try/catch around readFile and
page.addScriptTag so failures in loading the reverse rename tables are surfaced
immediately rather than ignored, and keep the fix localized to the
token-renames.js bootstrap path in the e2e harness.
- Around line 589-607: The real-page override in the e2e flow is only removed on
the success path, so a failing assertion can leave the injected stylesheet
behind. Update the test around applyRealPageOverride(), realPageSnapshot(), and
removeRealPageOverride() to ensure cleanup always runs by moving the revert
logic into a finally block that executes after the assertions. Keep the existing
before/after/reverted checks, but make the override removal unconditional so the
CDP-connected tab is restored even when focusedDiffs or deep equality assertions
fail.
In `@packages/dialtone-migration-helper/popup.js`:
- Around line 203-261: The toggle click handlers in popup.js are treating failed
content-script responses as success because they only test response?.error.
Update overrideBtn, avatarSwapBtn, and replaceBtn listeners to also handle
response?.ok === false (including skipped race-guard responses and override
failures with a report but no error) before setting state.dtOverride,
state.dtAvatarSwap, or state.dtReplace and before calling
setTabState/renderToggles. Keep the existing report rendering paths in
renderOverrideReport and renderAvatarReport, but ensure any non-ok response
exits early and does not persist the toggle as enabled.
In `@packages/dialtone-migration-helper/README.md`:
- Around line 24-28: Clarify the documentation around the DOM-rewrite boundary
in the README: the current wording in the safe token-only section conflicts with
the later note about rules.js and structure.js being loaded. Update the
explanation near the Avatar/Presence example to distinguish that the token
override path does not rewrite DOM structure, while the Avatar/Presence flow can
load structure.js for its incremental component swap behavior; use the existing
README sections mentioning the manifest, rules.js, structure.js, and
Avatar/Presence to make the boundary explicit.
- Around line 58-62: The README example in the real authenticated Firespotter
validation section exposes an internal beta URL with opaque identifiers, so
replace that `REAL_PAGE_URL` value with a generic placeholder. Update the
example in the `dialtone-migration-helper` README to keep the `REAL_PAGE_CDP`
and `--real-page` usage, but use a non-sensitive sample URL or placeholder token
instead of the current production-like path.
In `@packages/dialtone-migration-helper/structure.js`:
- Around line 248-276: The scheduling logic in scheduleReapply currently drops
mutation batches whenever pendingRaf is already set, so newly added candidates
can be missed until another mutation arrives. Update scheduleReapply (and the
related callback path around applyRulesToCandidates) to always collect and merge
unique candidates into a shared pending set/list even while a frame is pending,
then have the requestAnimationFrame callback process the accumulated candidates
and clear the pending collection.
- Around line 43-56: The current inverse for `replaceClass` in `structure.js` is
lossy because it only swaps prefixes back and can’t restore the exact pre-change
class list when `op.to` already exists or multiple source classes collapse.
Update `replaceClass` to capture and return the original class list snapshot as
the inverse payload, and make the inverse application restore that exact
snapshot instead of using `{ from: op.to, to: op.from }`; use the `replaceClass`
case and its inverse handling to locate the change.
In `@packages/dialtone-migration-helper/token-renames.js`:
- Around line 111-115: The layout-to-size fallback in token-renames.js drops the
`variant` when building candidates, so `-negative` tokens can map to positive
`--dt-size-*` values. Update the `SIZE_FROM_LAYOUT` handling in the `layout-* →
size-*` branch to preserve the `-negative` suffix when `variant ===
'-negative'`, while still keeping the base-token behavior for non-variant cases.
Use the existing `base`, `variant`, and `candidates.push(...)` flow to make the
reverse mapping carry the negative variant through correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 83f1007b-8cd4-4e6d-84f4-0f9885770697
📒 Files selected for processing (20)
eslint.config.jspackages/dialtone-migration-helper/.gitignorepackages/dialtone-migration-helper/README.mdpackages/dialtone-migration-helper/background.jspackages/dialtone-migration-helper/content.jspackages/dialtone-migration-helper/docs/analysis/components.jsonpackages/dialtone-migration-helper/docs/analysis/dependency-graph.jsonpackages/dialtone-migration-helper/docs/analysis/tokens.jsonpackages/dialtone-migration-helper/docs/analysis/utilities.jsonpackages/dialtone-migration-helper/e2e.mjspackages/dialtone-migration-helper/manifest.jsonpackages/dialtone-migration-helper/package.jsonpackages/dialtone-migration-helper/popup.csspackages/dialtone-migration-helper/popup.htmlpackages/dialtone-migration-helper/popup.jspackages/dialtone-migration-helper/rules.jspackages/dialtone-migration-helper/scripts/capture-components.jspackages/dialtone-migration-helper/scripts/diff-captures.mjspackages/dialtone-migration-helper/structure.jspackages/dialtone-migration-helper/token-renames.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
dialpad/ios(manual)dialpad/firespotter(manual)dialpad/semantic-release-changelog-json(auto-detected)dialpad/conventional-changelog-angular(auto-detected)
…ix stale docs Removes --real-page validation (findRealPage, applyRealPageOverride, runRealPageValidation, and its dedicated helpers) — never wired into CI or npm scripts, and its hardcoded default target was a personal, environment-specific internal URL that couldn't generalize to any other engineer or environment, so it never provided a repeatable regression check. Also fixes the content.js header comment (stale single-toggle description; now documents all four toggles and per-tab session storage), and removes the e2e.mjs try/catch that silently swallowed a token-renames.js load failure now that the file is a required content script.
- content.js: remove undeclared stableTheme assignment (threw ReferenceError under strict mode on every loadStableMap call); narrow captureOverrideSnapshot to real d- utility classes before the layout-forcing visibility filter. - eslint.config.js: re-enable no-undef for the package (was masking the above); verified zero violations. - background.js: cache 'latest' token maps with a 1-day TTL instead of forever, so the extension stops diffing against a stale release without a manual refresh. - token-renames.js: carry -negative through the layout->size reverse mapping, matching the sibling branches and the comment. - structure.js: accumulate mutation candidates across bursts within one frame (was dropping every batch after the first); restore the exact original class list on replaceClass revert instead of a lossy prefix swap-back. - popup.js: treat ok:false (race-skip) responses as not-applied so toggles do not flip on when nothing changed; broadcast toggles to all frames so they apply inside iframes (e.g. Storybook), not just the top document. - e2e.mjs: add TS-ST1 (burst merge) and TS-ST2 (exact class-list revert). - pnpm-lock.yaml: add the workspace importer entry so frozen-lockfile installs. - README.md: correct the DOM-rewrite boundary (structure engine is loaded, used only by the Avatar+Presence spike) and drop stale real-page references.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/dialtone-migration-helper/structure.js (1)
286-294: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
applyingwith try/finally — a throw here permanently deafens the observer.If
applyRulesToCandidatesthrows,applying = falseon line 293 never runs, so every subsequentscheduleReapplyearly-returns at line 267 and mutation remapping is dead for the rest of the session (untildisable()).🛡️ Proposed fix
applying = true; - applyRulesToCandidates(activeRules, batch); - applying = false; + try { + applyRulesToCandidates(activeRules, batch); + } finally { + applying = false; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dialtone-migration-helper/structure.js` around lines 286 - 294, The requestAnimationFrame callback in `scheduleReapply` leaves `applying` stuck true if `applyRulesToCandidates` throws, which prevents future remaps from being scheduled. Wrap the `pendingRaf` callback’s `applyRulesToCandidates(activeRules, batch)` call in a `try/finally` so `applying` is always reset to false, and keep the existing `pendingRaf`, `pendingCandidates`, and `pendingSeen` reset flow intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/dialtone-migration-helper/background.js`:
- Line 90: `getStableFrameworkCss()` is still returning cached framework CSS
without honoring the same `latest` TTL logic used for tokens, so align its cache
check with `getStableTokens()` by requiring freshness via `isCacheFresh(version,
cached)` before reusing cached `frameworkCss`. Update the cache guard in
`background.js` so the `latest` framework CSS is revalidated instead of being
returned unconditionally, keeping it in sync with the token baseline.
In `@packages/dialtone-migration-helper/popup.js`:
- Around line 113-118: The sendTab helper currently relies on
chrome.tabs.sendMessage with allFrames enabled, which can resolve from an
arbitrary frame and make setOverride, setAvatarSwap, and setReplace treat the
result as nondeterministic. Update sendTab in popup.js to aggregate responses
from all frames instead of using the single callback result, and make the caller
logic use the collected per-frame outcomes so the popup only reports not applied
when all relevant frames fail. Keep the fix localized to sendTab and the
handlers that depend on its returned status.
---
Duplicate comments:
In `@packages/dialtone-migration-helper/structure.js`:
- Around line 286-294: The requestAnimationFrame callback in `scheduleReapply`
leaves `applying` stuck true if `applyRulesToCandidates` throws, which prevents
future remaps from being scheduled. Wrap the `pendingRaf` callback’s
`applyRulesToCandidates(activeRules, batch)` call in a `try/finally` so
`applying` is always reset to false, and keep the existing `pendingRaf`,
`pendingCandidates`, and `pendingSeen` reset flow intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 4304c897-e74d-4b41-b1aa-3f47ca1e275e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by**
📒 Files selected for processing (8)
eslint.config.jspackages/dialtone-migration-helper/README.mdpackages/dialtone-migration-helper/background.jspackages/dialtone-migration-helper/content.jspackages/dialtone-migration-helper/e2e.mjspackages/dialtone-migration-helper/popup.jspackages/dialtone-migration-helper/structure.jspackages/dialtone-migration-helper/token-renames.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
dialpad/ios(manual)dialpad/firespotter(manual)dialpad/semantic-release-changelog-json(auto-detected)dialpad/conventional-changelog-angular(auto-detected)
💤 Files with no reviewable changes (1)
- eslint.config.js
| if (!brand || !mode) return { error: 'missing brand/mode' }; | ||
| const key = cacheKey(version, brand, mode); | ||
| const cached = (await chrome.storage.local.get(key))[key]; | ||
| if (cached && cached.map && cached.tokenCss && isCacheFresh(version, cached)) return cached; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C4 'getStableFrameworkCss|cacheKeyFw|isCacheFresh' packages/dialtone-migration-helper/background.jsRepository: dialpad/dialtone
Length of output: 2095
Apply the same latest TTL to framework CSS. getStableTokens() already revalidates latest, but getStableFrameworkCss() still returns any cached frameworkCss entry unconditionally. That lets dtFrameworkCss:latest stay pinned to an older release and drift from the token baseline used for diffs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/dialtone-migration-helper/background.js` at line 90,
`getStableFrameworkCss()` is still returning cached framework CSS without
honoring the same `latest` TTL logic used for tokens, so align its cache check
with `getStableTokens()` by requiring freshness via `isCacheFresh(version,
cached)` before reusing cached `frameworkCss`. Update the cache guard in
`background.js` so the `latest` framework CSS is revalidated instead of being
returned unconditionally, keeping it in sync with the token baseline.
| function sendTab(tabId, msg, { allFrames = false } = {}) { | ||
| return new Promise((resolve) => { | ||
| const timeout = setTimeout(() => resolve({ ok: false, error: 'tab response timed out' }), 10000); | ||
| const options = allFrames ? {} : { frameId: 0 }; | ||
| try { | ||
| chrome.tabs.sendMessage(tabId, msg, options, (r) => { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
chrome.tabs.sendMessage no frameId multiple frames response callback only first
💡 Result:
When using chrome.tabs.sendMessage, the API is designed for a single-message, single-response interaction [1][2]. If you send a message to a tab that contains multiple frames (and you do not specify a frameId), or if multiple listeners exist, the promise returned by sendMessage will resolve to only the first response received [3][2][4]. All subsequent responses are ignored [4]. To handle scenarios where you need to communicate with multiple frames individually, you should use the frameId parameter [5][2]. 1. Target Specific Frames: Use the frameId option in the options object to send the message to a specific frame [5][2]. chrome.tabs.sendMessage(tabId, message, { frameId: targetFrameId }); 2. Establish Communication Channels: If you need to maintain bi-directional communication or handle multiple responses, consider using runtime.connect to open long-lived ports instead of the one-time sendMessage API [6][7]. This allows you to create a unique connection for each frame, avoiding the race condition or response-dropping behavior associated with the single-promise resolution of sendMessage [6]. 3. Retrieve Frame IDs: If you need to dynamically discover frames to target them, use the chrome.webNavigation.getAllFrames or chrome.webNavigation.onCompleted events to map frame IDs within a tab [5]. In summary, sendMessage is inherently limited to a single response resolution [2][4]. For complex, multi-frame architectures where you expect responses from more than one frame, move away from sendMessage toward port-based messaging (chrome.runtime.connect) [6].
Citations:
- 1: https://developer.chrome.com/docs/extensions/develop/concepts/messaging
- 2: https://docs.w3cub.com/web_extensions/api/tabs/sendmessage
- 3: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/sendMessage
- 4: https://github.com/googlechrome/modern-web-guidance/blob/main/skills/chrome-extensions/references/extensions/message-passing.md
- 5: https://stackoverflow.com/questions/49542628/chrome-extension-sending-a-message-to-the-page-loaded-in-a-specific-iframe
- 6: https://stackoverflow.com/questions/70982073/chrome-tabs-sendmessage-not-working-with-background-script-and-iframe
- 7: https://stackoverflow.com/questions/31825978/chrome-tabs-sendmessage-not-working-while-sendresponse-works
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/dialtone-migration-helper/popup.js"
wc -l "$file"
sed -n '100,290p' "$file" | cat -nRepository: dialpad/dialtone
Length of output: 8093
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "setOverride|setAvatarSwap|setReplace|getOverrideReport|getAvatarSwapReport|setInspect|getDetectedTheme" packages/dialtone-migration-helperRepository: dialpad/dialtone
Length of output: 3927
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1360,1435p' packages/dialtone-migration-helper/content.js | cat -nRepository: dialpad/dialtone
Length of output: 3096
allFrames toggles can still resolve from the wrong frame
chrome.tabs.sendMessage(..., {}) only returns one frame’s reply, so the setOverride / setAvatarSwap / setReplace handlers can gate on a nondeterministic responder. If the top frame errors while the Storybook iframe succeeds, the popup still shows not applied and skips the state update.
Aggregate per-frame responses instead of trusting the broadcast callback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/dialtone-migration-helper/popup.js` around lines 113 - 118, The
sendTab helper currently relies on chrome.tabs.sendMessage with allFrames
enabled, which can resolve from an arbitrary frame and make setOverride,
setAvatarSwap, and setReplace treat the result as nondeterministic. Update
sendTab in popup.js to aggregate responses from all frames instead of using the
single callback result, and make the caller logic use the collected per-frame
outcomes so the popup only reports not applied when all relevant frames fail.
Keep the fix localized to sendTab and the handlers that depend on its returned
status.
|
close without merge? revisit? |
What
Lands the
packages/dialtone-migration-helperChrome extension into the monorepo (initial commit — the package existed on disk but was untracked).Changes
Replace mode (was broken)
The previous implementation crammed the full stable framework CSS into the first
dialtone-css-*tag and blanked the other two, destroying Next's runtime theming API slots and leaving tokens undefined. Rebuilt as in-place value substitution:[data-dt-mode]),@layerwrappers, and token names byte-for-byte--dt-*declaration values are swapped to the last stable releasetoken-renames.js(spacing←space,layout←size,positive←success, border/radius)setMode()/setBrand()keep working natively — no MutationObserver hack neededToken inspector tooltip
Utility class section now shows custom-property declarations one per line (capped at 6 with
…+N more) instead of dumping the rawcssTextblob.ESLint
Added browser + Chrome extension globals override in
eslint.config.jsfor the migration helper package.How to test
packages/dialtone-migration-helperunpacked inchrome://extensionsd-*classes, utility declarations show one per line