Shared frontend layer - app & webflow#298
Open
simonsmallchua wants to merge 49 commits into
Open
Conversation
|
Updates to Preview Branch (feat/shared-extension-site-jobs) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
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:
📝 WalkthroughWalkthroughThis PR centralises job and Webflow integration logic into shared frontend modules, exposes them to the Webflow Designer extension via the bridge, and refactors both the main app and extension to delegate fetching, domain scoping, signature building and realtime subscription orchestration to those shared modules. Documentation and planning files were added/updated to reflect the ES modules migration and a hybrid auth popup flow. Changes
Sequence DiagramssequenceDiagram
participant Extension as Webflow Extension
participant Bridge as Bridge (window.HoverLib)
participant SharedJobs as Shared Jobs Module
participant Supabase as Supabase Realtime
participant Polling as Polling Fallback
Extension->>Bridge: HoverLib.jobs.subscribeToJobUpdates({ orgId, onUpdate })
Bridge->>SharedJobs: subscribeToJobUpdates(...)
SharedJobs->>Supabase: subscribe postgres_changes (jobs)
SharedJobs->>Polling: start polling fallback immediately
Supabase-->>SharedJobs: realtime event
SharedJobs->>Polling: clear fallback on first realtime event
SharedJobs->>Extension: invoke onUpdate()
Note right of SharedJobs: on subscription error -> call onSubscriptionIssue and continue polling
sequenceDiagram
participant Extension as Webflow Extension
participant Popup as Auth Popup (webflow-login.js)
participant LegacyAuth as Legacy Auth (/js/auth.js)
participant Server as Backend API
Extension->>Popup: open popup with origin & state params
Popup->>Popup: validate opener, state, referrer
Popup->>LegacyAuth: loadAuthModalFromLegacy()
LegacyAuth->>Server: OAuth flow / token exchange
Server-->>LegacyAuth: auth response
LegacyAuth->>Popup: authentication complete
Popup->>LegacyAuth: registerUserWithBackend(user)
LegacyAuth->>Server: POST /v1/auth/register
Server-->>Popup: registration result
Popup->>Extension: postMessage({ source, state, extensionState, accessToken }, targetOrigin)
Extension->>Extension: validate state/origin and save token
Possibly related PRs
🚥 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. Comment |
Contributor
Release VersionsApp patch: ChangelogAdded
Changed
Fixed
|
![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: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 33-35: The changelog bullet is missing a verb; update the first
line in CHANGELOG.md to read explicitly (use Australian English) by replacing
the fragment starting with "shared Webflow extension job fetching..." with a
full statement such as "Centralised Webflow extension job fetching, site
scoping, and realtime fallback logic into `web/static/app/lib/site-jobs.js`,
reducing duplication between the app layer and extension bridge runtime" so the
release note is a complete sentence and uses Australian spelling
("Centralised").
In `@docs/plans/webflow-extension-reuse-follow-up.md`:
- Around line 71-72: Update the sentence about removing the legacy auth
dependency in /extension-auth to explicitly preserve the centralized OAuth
redirect contract: state that the popup shell may be migrated but OAuth
redirects must remain centralized in web/static/js/auth.js via
handleSocialLogin, and deep-link URLs must return to the exact originating URL;
ensure the text mentions preserving the redirect handler (web/static/js/auth.js
/ handleSocialLogin) and the requirement that deep-link URLs restore the
original URL rather than dropping that behavior.
- Around line 18-24: Update the "current-state" inventory to explicitly call out
the shared jobs helper by name: mention web/static/app/lib/site-jobs.js
(site-jobs.js) as the centralised shared implementation that handles fetch,
site-scoping, and realtime fallback for both the extension and main app; ensure
it is listed separately from the generic "app/lib/" bullet so reviewers know
refresh/job logic is already shared rather than local to the extension.
In `@web/static/app/lib/site-jobs.js`:
- Around line 120-129: The buildChartJobsSignature function currently ignores
per-job stats used to render bars, so chart signatures remain unchanged when
stats like total_broken_links or slow_page_buckets update; update
buildChartJobsSignature to include the relevant stats from job.stats (at minimum
job.stats.total_broken_links and job.stats.slow_page_buckets, and any other
stats used by the chart-building logic) in the returned signature string for
each job (e.g., append or interleave these stat values alongside
job.id/status/failed_tasks) so lastChartJobsSignature will change when these
stats change.
- Around line 291-292: The code currently calls startFallback() on every
successful subscribe(), causing polling to be armed repeatedly; change the logic
so startFallback() is only invoked when the realtime subscription actually fails
(i.e., in the subscription failure/error callback or after a connection retry
fails), or guard it with a boolean (e.g., fallbackArmed) so subscribe() does not
re-arm polling on success; ensure clearFallback() remains called on the first
real event to stop polling and that startFallback()/clearFallback() refs are
used (not duplicated) so polling only starts after a realtime failure.
In `@web/static/app/pages/webflow-jobs.js`:
- Around line 145-152: The call to subscribeToSharedJobUpdates is passing a
single options object so the real onUpdate argument becomes undefined; update
the call to use positional arguments so onUpdate is passed correctly (e.g. call
subscribeToSharedJobUpdates(orgId, onUpdate, { getFallbackInterval: () =>
window.dataBinder?.hasRealtimeActiveJobs ? FALLBACK_POLLING_INTERVAL_ACTIVE_MS :
FALLBACK_POLLING_INTERVAL_IDLE_MS })). Ensure you reference the existing symbols
subscribeToSharedJobUpdates, orgId, onUpdate and the getFallbackInterval option
so the fallback timer will call the real onUpdate handler.
In `@webflow-designer-extension-cli/scripts/sync-shared.js`:
- Line 34: The sync script currently treats "lib/site-jobs.js" as optional which
lets builds succeed but runtime import from public/lib/bridge.js fail; update
scripts/sync-shared.js to distinguish required vs optional targets (e.g., create
REQUIRED_LIB_FILES array containing "lib/site-jobs.js" and keep optional list
separate) and alter the sync flow to treat missing files in REQUIRED_LIB_FILES
as fatal by logging the error and exiting non-zero (or throwing) instead of
warning and skipping; ensure any existing warn-and-skip logic is only applied to
the optional list and that the public/lib/bridge.js dependency is documented as
required.
In `@webflow-designer-extension-cli/src/index.ts`:
- Around line 769-793: When the realtime subscription is established via
HoverLib.jobs.subscribeToJobUpdates (the block that calls realtimeRefresh()),
stop the legacy poller so active jobs aren't polled twice: clear and null out
the jobStatusPoller interval (or call the existing stop/cleanup helper for it)
as part of subscription setup/success (for example inside the subscribe
callback/onUpdate or immediately after jobsSubscriptionCleanup is assigned).
Ensure you reference and update the jobStatusPoller symbol so the interval is
cancelled and will not continue alongside the new realtime subscription/fallback
polling.
- Around line 763-793: The function subscribeToJobUpdates is unnecessarily
declared async (no await inside) which triggers require-await and turns
synchronous errors into rejected promises; remove the async keyword so it
becomes a plain synchronous function that returns void, keeping the existing
body (including references to state.activeOrganisationId,
cleanupRealtimeSubscription, jobsSubscriptionCleanup,
HoverLib.jobs.subscribeToJobUpdates, and realtimeRefresh) unchanged so callers
(e.g., void subscribeToJobUpdates()) behave as intended.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a9e3450-b66a-42c4-b6b3-daeb1343a5f5
📒 Files selected for processing (10)
CHANGELOG.mddocs/architecture/webflow-designer-extension.mddocs/plans/README.mddocs/plans/webflow-extension-reuse-follow-up.mdweb/static/app/lib/site-jobs.jsweb/static/app/pages/webflow-jobs.jswebflow-designer-extension-cli/README.mdwebflow-designer-extension-cli/public/lib/bridge.jswebflow-designer-extension-cli/scripts/sync-shared.jswebflow-designer-extension-cli/src/index.ts
Comment on lines
+145
to
+152
| return subscribeToSharedJobUpdates({ | ||
| orgId, | ||
| onUpdate, | ||
| getFallbackInterval: () => | ||
| window.dataBinder?.hasRealtimeActiveJobs | ||
| ? FALLBACK_POLLING_INTERVAL_ACTIVE_MS | ||
| : FALLBACK_POLLING_INTERVAL_IDLE_MS, | ||
| }); |
Contributor
There was a problem hiding this comment.
This wrapper is calling the shared subscription helper with the wrong shape.
web/static/app/lib/site-jobs.js exposes subscribeToJobUpdates(jobID, onUpdate, options), but this call now passes a single object. That leaves the real onUpdate argument undefined, so the fallback timer will eventually call undefined() and live refresh recovery breaks. Either call the shared helper positionally, or expose a separate org-scoped API from site-jobs.js.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/static/app/pages/webflow-jobs.js` around lines 145 - 152, The call to
subscribeToSharedJobUpdates is passing a single options object so the real
onUpdate argument becomes undefined; update the call to use positional arguments
so onUpdate is passed correctly (e.g. call subscribeToSharedJobUpdates(orgId,
onUpdate, { getFallbackInterval: () => window.dataBinder?.hasRealtimeActiveJobs
? FALLBACK_POLLING_INTERVAL_ACTIVE_MS : FALLBACK_POLLING_INTERVAL_IDLE_MS })).
Ensure you reference the existing symbols subscribeToSharedJobUpdates, orgId,
onUpdate and the getFallbackInterval option so the fallback timer will call the
real onUpdate handler.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
web/static/app/lib/site-jobs.js (2)
286-297:⚠️ Potential issue | 🟠 MajorDo not arm the polling fallback on a healthy subscribe.
startFallback()still runs unconditionally after.subscribe(), so an idle but healthy subscription refreshes every second until some job event arrives. That keeps both surfaces doing background work even when realtime is fine. If you still need protection against “subscribed but silent” environments, gate it behind an explicit watchdog instead of leaving the interval on for every page.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/static/app/lib/site-jobs.js` around lines 286 - 297, The fallback polling is being started unconditionally after calling .subscribe(), causing unnecessary polling even when the subscription is healthy; remove the unconditional startFallback() call and only invoke startFallback() from the subscribe error branch (where status === "CHANNEL_ERROR" || status === "TIMED_OUT" || err) or behind an explicit watchdog flag/option if you need protection for "subscribed but silent" cases; ensure the existing clearFallback() is still called on the first real event to stop polling.
120-135:⚠️ Potential issue | 🟠 MajorInclude chart-driving stats in the signature.
renderMiniChart()inwebflow-designer-extension-cli/src/index.tsbuilds each bar fromjob.stats.total_broken_linksandjob.stats.slow_page_buckets, but this signature ignores both. If those stats arrive or are corrected after the first fetch, the signature does not change and the chart stays stale.🐛 Proposed fix
return chartJobs - .map( - (job) => - `${job.id || ""}:${job.status || ""}:${job.failed_tasks || 0}:${job.skipped_tasks || 0}:${job.completed_at || ""}:${job.total_tasks || 0}` - ) + .map((job) => { + const buckets = job.stats?.slow_page_buckets || {}; + return [ + job.id || "", + job.status || "", + job.failed_tasks || 0, + job.skipped_tasks || 0, + job.completed_at || "", + job.total_tasks || 0, + job.stats?.total_broken_links || 0, + buckets.over_10s || 0, + buckets["5_to_10s"] || 0, + buckets["3_to_5s"] || 0, + ].join(":"); + }) .join("|");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/static/app/lib/site-jobs.js` around lines 120 - 135, buildChartJobsSignature currently omits chart-driving fields so charts can stay stale; update the signature generation inside buildChartJobsSignature (the .map that produces the `${job.id...}` string) to also include job.stats.total_broken_links and job.stats.slow_page_buckets (or a stable serialization of those arrays/objects) along with the existing fields so changes to those stats will change the returned signature used by renderMiniChart.webflow-designer-extension-cli/src/index.ts (1)
769-793:⚠️ Potential issue | 🟠 MajorStop the legacy poller before handing off to the shared subscription.
loadLatestJob()can already have armedjobStatusPollerearlier in the same refresh. Because this hand-off only setsjobsSubscriptionCleanup, the existing 6 s interval keeps running alongsideHoverLib.jobs.subscribeToJobUpdates()until the job leaves an active state.♻️ Proposed fix
cleanupRealtimeSubscription(); + stopJobStatusPolling(); jobsSubscriptionCleanup = HoverLib.jobs.subscribeToJobUpdates({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webflow-designer-extension-cli/src/index.ts` around lines 769 - 793, Before assigning jobsSubscriptionCleanup, ensure the legacy 6s poller is stopped so it doesn't run alongside HoverLib.jobs.subscribeToJobUpdates: call the existing cleanupRealtimeSubscription() (or, if the legacy poller is tracked by a variable like jobStatusPoller, call clearInterval(jobStatusPoller) or a dedicated stopJobStatusPoller() helper) immediately before invoking HoverLib.jobs.subscribeToJobUpdates in realtimeRefresh()/loadLatestJob(); if no stop helper exists, add one that clears the interval and nulls the poller reference and invoke it prior to setting jobsSubscriptionCleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@web/static/app/lib/site-jobs.js`:
- Around line 286-297: The fallback polling is being started unconditionally
after calling .subscribe(), causing unnecessary polling even when the
subscription is healthy; remove the unconditional startFallback() call and only
invoke startFallback() from the subscribe error branch (where status ===
"CHANNEL_ERROR" || status === "TIMED_OUT" || err) or behind an explicit watchdog
flag/option if you need protection for "subscribed but silent" cases; ensure the
existing clearFallback() is still called on the first real event to stop
polling.
- Around line 120-135: buildChartJobsSignature currently omits chart-driving
fields so charts can stay stale; update the signature generation inside
buildChartJobsSignature (the .map that produces the `${job.id...}` string) to
also include job.stats.total_broken_links and job.stats.slow_page_buckets (or a
stable serialization of those arrays/objects) along with the existing fields so
changes to those stats will change the returned signature used by
renderMiniChart.
In `@webflow-designer-extension-cli/src/index.ts`:
- Around line 769-793: Before assigning jobsSubscriptionCleanup, ensure the
legacy 6s poller is stopped so it doesn't run alongside
HoverLib.jobs.subscribeToJobUpdates: call the existing
cleanupRealtimeSubscription() (or, if the legacy poller is tracked by a variable
like jobStatusPoller, call clearInterval(jobStatusPoller) or a dedicated
stopJobStatusPoller() helper) immediately before invoking
HoverLib.jobs.subscribeToJobUpdates in realtimeRefresh()/loadLatestJob(); if no
stop helper exists, add one that clears the interval and nulls the poller
reference and invoke it prior to setting jobsSubscriptionCleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e8ca8299-cc88-4df7-810d-1fe78ec5ef1b
📒 Files selected for processing (5)
.github/workflows/fly-deploy.ymlCHANGELOG.mddocs/plans/webflow-extension-reuse-follow-up.mdweb/static/app/lib/site-jobs.jswebflow-designer-extension-cli/src/index.ts
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
web/static/app/lib/site-jobs.js (1)
286-297:⚠️ Potential issue | 🟠 MajorDon't arm fallback polling on a healthy realtime subscription.
This still starts the fallback interval immediately after every
subscribe(). In the extension, that means the sharedonUpdatepath keeps refreshing/v1/jobsand/v1/usageevery second until the first realtime event arrives, even when the channel is healthy. Start fallback only from the error/retry-exhausted path, and clear it onSUBSCRIBED.Proposed fix
.subscribe((status, err) => { + if (status === "SUBSCRIBED") { + clearFallback(); + return; + } if ( (status === "CHANNEL_ERROR" || status === "TIMED_OUT" || err) && !unsubscribed ) { onSubscriptionIssue?.(status, err); startFallback(); } }); - - // Start fallback immediately; clearFallback() stops it on the first real event. - startFallback();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/static/app/lib/site-jobs.js` around lines 286 - 297, The code currently calls startFallback() immediately after subscribe(), causing fallback polling even when the realtime channel is healthy; change the flow so startFallback() is invoked only from the error/retry-exhausted path inside the subscribe callback (e.g., when status === "CHANNEL_ERROR" || status === "TIMED_OUT" || err or a specific "RETRY_EXHAUSTED" state) and remove the unconditional startFallback() call after subscribe(); also ensure clearFallback() is called when the subscribe callback reports status === "SUBSCRIBED" so the fallback interval is cancelled as soon as the channel becomes healthy (refer to the subscribe callback, startFallback, clearFallback, onSubscriptionIssue and handling of the "SUBSCRIBED" status).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/static/app/lib/settings/integrations/webflow.js`:
- Line 202: listWebflowSites() is paginated and returns only one backend page
(with pagination metadata), but the code stores that single page into
sitesState.sites and treats it as the full collection; update the logic to
either (A) exhaustively fetch all pages from listWebflowSites() before assigning
to sitesState.sites (looping on data.pagination.next or page cursors and
concatenating results), or (B) keep sitesState.sites as just the current page
and drive client next/prev/search by using data.pagination and calling
listWebflowSites(connectionId, { page }) on navigation/search; adjust the code
around listWebflowSites, sitesState.sites and any client paging/search handlers
to use one of these two approaches so later records beyond the first backend
page are reachable.
In `@web/static/app/pages/webflow-login.js`:
- Around line 269-291: Fail closed and restrict allowed preview hosts before
calling handleAuthenticated(): if window.GNHAuth or its
isValidExtensionTargetOrigin is missing, treat validation as failed and return
an error via setStatus; use
window.GNHAuth.isValidExtensionTargetOrigin(TARGET_ORIGIN) to gate continuation.
Additionally, add an explicit allowlist check for permitted preview origins
(instead of allowing any *.fly.dev) when validating referrer/document.referrer
and TARGET_ORIGIN—compare the referrer origin against a hardcoded project-owned
list and reject if not present. Ensure all failures return false via setStatus
and do not proceed to handleAuthenticated(), and do not log or expose any tokens
or secrets in these messages.
- Around line 240-243: The wrapper async function loadAuthModalFromLegacy
currently awaits window.GNHAuth.loadAuthModal() but treats any falsy/false
return as success; change it to detect a false (or other failure sentinel)
return from window.GNHAuth.loadAuthModal() and propagate the failure by either
throwing an Error or returning false so callers (e.g., the sign-in flow that
calls loadAuthModalFromLegacy) do not continue into a broken state; update
loadAuthModalFromLegacy to check the result of window.GNHAuth.loadAuthModal()
and immediately throw or return a failure value when it indicates failure.
---
Duplicate comments:
In `@web/static/app/lib/site-jobs.js`:
- Around line 286-297: The code currently calls startFallback() immediately
after subscribe(), causing fallback polling even when the realtime channel is
healthy; change the flow so startFallback() is invoked only from the
error/retry-exhausted path inside the subscribe callback (e.g., when status ===
"CHANNEL_ERROR" || status === "TIMED_OUT" || err or a specific "RETRY_EXHAUSTED"
state) and remove the unconditional startFallback() call after subscribe(); also
ensure clearFallback() is called when the subscribe callback reports status ===
"SUBSCRIBED" so the fallback interval is cancelled as soon as the channel
becomes healthy (refer to the subscribe callback, startFallback, clearFallback,
onSubscriptionIssue and handling of the "SUBSCRIBED" status).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f45b1319-ad5e-4f07-a7d3-990d93f964b3
📒 Files selected for processing (8)
web/static/app/lib/settings/integrations/webflow.jsweb/static/app/lib/site-jobs.jsweb/static/app/lib/webflow-sites.jsweb/static/app/pages/webflow-login.jsweb/static/js/auth.jswebflow-designer-extension-cli/public/lib/bridge.jswebflow-designer-extension-cli/scripts/sync-shared.jswebflow-designer-extension-cli/src/index.ts
|
|
||
| const json = await response.json(); | ||
| const data = json?.data ?? { sites: [] }; | ||
| const data = await listWebflowSites(connectionId, { page }); |
Contributor
There was a problem hiding this comment.
Don't treat a paginated helper as if it returned the full site list.
listWebflowSites() returns one backend page (web/static/app/lib/webflow-sites.js defaults to 50 items and exposes pagination), but this module still feeds sitesState.sites into client-side paging and search as if it were the complete collection. Any workspace with more than 50 sites will never surface or search later records. Either exhaust backend pages before storing sitesState.sites, or move next/prev/search to data.pagination and fetch server pages explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/static/app/lib/settings/integrations/webflow.js` at line 202,
listWebflowSites() is paginated and returns only one backend page (with
pagination metadata), but the code stores that single page into sitesState.sites
and treats it as the full collection; update the logic to either (A)
exhaustively fetch all pages from listWebflowSites() before assigning to
sitesState.sites (looping on data.pagination.next or page cursors and
concatenating results), or (B) keep sitesState.sites as just the current page
and drive client next/prev/search by using data.pagination and calling
listWebflowSites(connectionId, { page }) on navigation/search; adjust the code
around listWebflowSites, sitesState.sites and any client paging/search handlers
to use one of these two approaches so later records beyond the first backend
page are reachable.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dashboard.html (1)
103-115: 🧹 Nitpick | 🔵 TrivialApply
forattributes consistently across all dashboard form fields.Similar to the Domain field, the Max Pages, Concurrency, and Schedule labels would benefit from explicit
forattributes linking them to their respective controls (dashboardMaxPages,dashboardConcurrency,dashboardScheduleInterval).🔧 Proposed fix
<div class="dashboard-field"> - <label class="dashboard-field__label">Max Pages</label> + <label class="dashboard-field__label" for="dashboardMaxPages">Max Pages</label> <input type="number" id="dashboardMaxPages"<div class="dashboard-field"> - <label class="dashboard-field__label">Concurrency</label> + <label class="dashboard-field__label" for="dashboardConcurrency">Concurrency</label> <select class="dashboard-field__control" id="dashboardConcurrency"<div class="dashboard-field"> - <label class="dashboard-field__label">Schedule</label> + <label class="dashboard-field__label" for="dashboardScheduleInterval">Schedule</label> <select class="dashboard-field__control" id="dashboardScheduleInterval"Also applies to: 117-141, 143-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dashboard.html` around lines 103 - 115, The label elements for several dashboard form fields are missing explicit for attributes so they're not linked to their inputs; update the labels to include for="dashboardMaxPages", for="dashboardConcurrency", and for="dashboardScheduleInterval" (and any other label controlling inputs referenced in the comment) so each label targets the matching input id (e.g., the Max Pages label -> dashboardMaxPages, Concurrency label -> dashboardConcurrency, Schedule label -> dashboardScheduleInterval) ensuring consistent accessibility across the form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dashboard.html`:
- Around line 91-100: The label element for the Domain input is not
programmatically associated with the input (id "dashboardDomain"), causing
accessibility tooling to flag a missing label; update the label element
(dashboard-field__label) to include for="dashboardDomain" so it is correctly
bound to the input with id="dashboardDomain" and improves screen reader
accessibility.
- Around line 214-227: The Sign In and Create Account buttons (elements with
id="showLoginBtn" and id="showSignupBtn") are missing an explicit type so they
default to type="submit" and may trigger form submission; update both <button>
elements to include type="button" to prevent unintended form submits while
preserving existing classes, aria-labels, and IDs.
In `@web/static/app/lib/organisation-api.js`:
- Around line 45-49: createOrganisation and switchOrganisation currently return
null when the API returns a 2xx response that lacks the expected organisation
property, losing any error text the server may have sent; update both functions
(createOrganisation, switchOrganisation) to inspect the response body when
response?.organisation is missing and, if the body contains an error/message
field, throw a descriptive error (or rethrow an ApiError) that includes that
server-provided message so callers can distinguish missing data from a
server-side error; use resolveApi and the existing api.post response to locate
the response object and preserve existing ApiError semantics when appropriate.
In `@web/static/app/lib/scheduler-api.js`:
- Line 83: The PUT/POST methods updateScheduler, disableScheduler, and
saveSchedulerForDomain build URLs using scheduler IDs without encoding, causing
inconsistency with getScheduler and deleteScheduler which use
encodeURIComponent; update those functions to wrap the
schedulerId/currentScheduler.id with encodeURIComponent (the same as
getScheduler/deleteScheduler) when constructing the URL (e.g.,
encodeURIComponent(currentScheduler.id) or encodeURIComponent(schedulerId)) so
all scheduler-related endpoints use consistent URL encoding.
---
Outside diff comments:
In `@dashboard.html`:
- Around line 103-115: The label elements for several dashboard form fields are
missing explicit for attributes so they're not linked to their inputs; update
the labels to include for="dashboardMaxPages", for="dashboardConcurrency", and
for="dashboardScheduleInterval" (and any other label controlling inputs
referenced in the comment) so each label targets the matching input id (e.g.,
the Max Pages label -> dashboardMaxPages, Concurrency label ->
dashboardConcurrency, Schedule label -> dashboardScheduleInterval) ensuring
consistent accessibility across the form.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 93575b12-5e4b-4590-ae50-2f2a0ff5a450
📒 Files selected for processing (10)
dashboard.htmlweb/static/app/lib/organisation-api.jsweb/static/app/lib/scheduler-api.jsweb/static/app/lib/settings/organisations.jsweb/static/app/lib/settings/schedules.jsweb/static/app/pages/dashboard.jsweb/static/app/styles/dashboard-extension.csswebflow-designer-extension-cli/public/lib/bridge.jswebflow-designer-extension-cli/scripts/sync-shared.jswebflow-designer-extension-cli/src/index.ts
Comment on lines
+45
to
+49
| export async function createOrganisation(name, options = {}) { | ||
| const api = resolveApi(options.api); | ||
| const response = await api.post("/v1/organisations", { name }); | ||
| return response?.organisation || null; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how api-client handles errors - does it throw on non-2xx?
rg -n -A 10 'export.*function.*post' web/static/app/lib/api-client.js
rg -n 'throw' web/static/app/lib/api-client.jsRepository: Harvey-AU/hover
Length of output: 477
Propagate server-side error messages when response has unexpected shape.
The createOrganisation and switchOrganisation functions return null when the API response lacks the expected organisation property. Since api.post() correctly throws ApiError on non-2xx responses, the scenario where a caller receives null is when the API returns a 2xx status but with an unexpected response structure (e.g., { error: "..." }). In this case, callers have no way to distinguish between missing data and an error message in the response body.
Consider extracting error information from 2xx responses with unexpected structures and propagating it to callers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/static/app/lib/organisation-api.js` around lines 45 - 49,
createOrganisation and switchOrganisation currently return null when the API
returns a 2xx response that lacks the expected organisation property, losing any
error text the server may have sent; update both functions (createOrganisation,
switchOrganisation) to inspect the response body when response?.organisation is
missing and, if the body contains an error/message field, throw a descriptive
error (or rethrow an ApiError) that includes that server-provided message so
callers can distinguish missing data from a server-side error; use resolveApi
and the existing api.post response to locate the response object and preserve
existing ApiError semantics when appropriate.
| }); | ||
| } | ||
|
|
||
| return api.put(`/v1/schedulers/${currentScheduler.id}`, { |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent URL encoding for schedulerId in PUT operations.
getScheduler (line 32) and deleteScheduler (line 121) encode the schedulerId with encodeURIComponent, but updateScheduler, disableScheduler, and saveSchedulerForDomain do not. While scheduler IDs are likely UUIDs without special characters, the inconsistency could cause issues if ID formats change.
♻️ Proposed fix to apply consistent encoding
- return api.put(`/v1/schedulers/${currentScheduler.id}`, {
+ return api.put(`/v1/schedulers/${encodeURIComponent(currentScheduler.id)}`, {
...payload,
is_enabled: true,
});- return api.put(`/v1/schedulers/${schedulerId}`, payload);
+ return api.put(`/v1/schedulers/${encodeURIComponent(schedulerId)}`, payload);- return api.put(`/v1/schedulers/${schedulerId}`, updates);
+ return api.put(`/v1/schedulers/${encodeURIComponent(schedulerId)}`, updates);Also applies to: 100-100, 111-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/static/app/lib/scheduler-api.js` at line 83, The PUT/POST methods
updateScheduler, disableScheduler, and saveSchedulerForDomain build URLs using
scheduler IDs without encoding, causing inconsistency with getScheduler and
deleteScheduler which use encodeURIComponent; update those functions to wrap the
schedulerId/currentScheduler.id with encodeURIComponent (the same as
getScheduler/deleteScheduler) when constructing the URL (e.g.,
encodeURIComponent(currentScheduler.id) or encodeURIComponent(schedulerId)) so
all scheduler-related endpoints use consistent URL encoding.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
web/static/app/pages/webflow-login.js (1)
240-246:⚠️ Potential issue | 🟠 MajorPropagate
loadAuthModal()’s failure sentinel.
window.GNHAuth.loadAuthModal()can resolvefalsewhen the modal fetch or injection fails. Line 242 still treats that as success, soinit()can continue intoshowLogin()with no modal mounted.Proposed fix
async function loadAuthModalFromLegacy() { if (typeof window.GNHAuth?.loadAuthModal === "function") { - await window.GNHAuth.loadAuthModal(); + const loaded = await window.GNHAuth.loadAuthModal(); + if (loaded === false) { + throw new Error("Auth modal failed to load"); + } return; } throw new Error("Auth modal loader unavailable"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/static/app/pages/webflow-login.js` around lines 240 - 246, The function loadAuthModalFromLegacy should treat a resolved false from window.GNHAuth.loadAuthModal() as failure; update loadAuthModalFromLegacy to await window.GNHAuth.loadAuthModal(), check the returned value, and if it is strictly false throw an Error (so callers like init() won't proceed to showLogin() without a mounted modal). Keep the existing fallback that throws when loadAuthModal is unavailable; specifically reference loadAuthModalFromLegacy and window.GNHAuth.loadAuthModal to locate and change the success handling to propagate the failure sentinel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@web/static/app/pages/webflow-login.js`:
- Around line 240-246: The function loadAuthModalFromLegacy should treat a
resolved false from window.GNHAuth.loadAuthModal() as failure; update
loadAuthModalFromLegacy to await window.GNHAuth.loadAuthModal(), check the
returned value, and if it is strictly false throw an Error (so callers like
init() won't proceed to showLogin() without a mounted modal). Keep the existing
fallback that throws when loadAuthModal is unavailable; specifically reference
loadAuthModalFromLegacy and window.GNHAuth.loadAuthModal to locate and change
the success handling to propagate the failure sentinel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 338d9c0b-ec8d-4f87-a838-6cb369231c5d
📒 Files selected for processing (2)
web/static/app/pages/webflow-login.jsweb/templates/extension-auth.html
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
Contributor
Author
|
Corrected handover summary Current state
Why the Gravatar issue happened
What still remains
Best next steps
|
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
simonsmallchua
changed the title
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-298.fly.dev |
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
This branch has moved well beyond the original jobs-only extraction. It now
pushes the Webflow extension and
/dashboardonto a much more shared frontendlayer, while still leaving a clear set of native-extension follow-up work.
Delivered in this branch
organisation context, schedulers, exports, shell navigation, and shared
site-view rendering under
web/static/app/lib/modules instead of keeping duplicate fetch/filter/realtime logic locally
/dashboardonto the extension-style shell/layout and sharedsite-focused module runtime instead of the older dashboard shell
web/static/app/styles/shell.css, nowloaded by both dashboard and extension
/extension-authflow inweb/static/app/pages/webflow-login.jsAccount) using sharedsettings logic inside the extension shell rather than loading the app page
Remaining work
Teamshould be next,then the remaining org-scoped settings sections
webflow-designer-extension-cli/src/index.tsis still too large and stillowns too much orchestration
extension still carries a large local stylesheet
but still duplicated rather than centralised into one shared helper
registration outside this frontend branch
Validation
Targeted checks run during this branch included:
cd webflow-designer-extension-cli && npm run sync-sharedcd webflow-designer-extension-cli && npm run compilecd webflow-designer-extension-cli && npm run lintprettier --check/prettier --writeon touched HTML/CSS/JS/MD filesgo test ./internal/api -run TestCORSMiddlewarego test ./internal/api -run 'TestSecurityHeadersMiddleware|TestMiddlewareChaining'