Repository navigation
Conversation
Dependency ReviewThe following issues were found:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## explorer_3_explorer_refactor #2058 +/- ##
================================================================
- Coverage 92.10% 92.07% -0.03%
================================================================
Files 82 83 +1
Lines 13186 13239 +53
Branches 1878 1884 +6
================================================================
+ Hits 12145 12190 +45
- Misses 1003 1011 +8
Partials 38 38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| }), | ||
| ) | ||
| .command('doctor', 'Check your set-up for potential problems') | ||
| .command('explore', 'Opens an interactive web explorer for your CDK app', (yargs: Argv) => |
There was a problem hiding this comment.
Mm. To my mind it's (mostly) cdk <verb>. So explore. But we can make an alias as well.
|
Automated review A maintainer will still review this — treat the notes below as a starting point. 📌 Commit This PR adds the 🔴 1 blocking · 🟡 2 recommended · ⚪ 2 optional Files with findings (5)Generated automatically. React 👍 or 👎 to tell us whether this review helped, so we can improve these reviews. |
![maintainer-for-aws[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| const onSignal = () => { | ||
| // eslint-disable-next-line no-console |
There was a problem hiding this comment.
⚪ Optional — The // eslint-disable-next-line no-console directive sits above void options.ioHelper.defaults.info('Stopping.'), which contains no console.* call — output correctly goes through the IoHost. The directive therefore suppresses nothing and is a stray artifact; if the lint config reports unused disable directives it is also a latent build break.
Suggested change: Delete the // eslint-disable-next-line no-console line above the options.ioHelper.defaults.info('Stopping.') call.
| * Serves the web explorer SPA from memory. The bytes come from | ||
| * web-assets.generated.json (written by build-tools/bundle-frontend.ts); the |
There was a problem hiding this comment.
⚪ Optional — The file header describes the embedded-assets artifact as web-assets.generated.json, but the code imports ./web-assets.generated, which resolves to a TypeScript file (web-assets.generated.ts) — the build writes and requires a .ts module, not JSON. The comment misdescribes the mechanism it documents, so a reader looking for a JSON artifact will not find one.
Suggested change: Change "web-assets.generated.json" to "web-assets.generated.ts" in the header comment.
| * Builds the web explorer SPA into lib/web/static, then writes the same assets | ||
| * to lib/web/web-assets.generated.json so they ride the require() graph into |
There was a problem hiding this comment.
⚪ Optional — The file header says it writes the assets to lib/web/web-assets.generated.json, but embeddedAssetsFile is set to ...web-assets.generated.ts and writeEmbeddedAssets emits a TypeScript module (export const EMBEDDED_ASSETS = {...}), not JSON. The comment misdescribes the output file it documents, which can mislead the next reader.
Suggested change: Change "web-assets.generated.json" to "web-assets.generated.ts" in the header comment.
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| 'explore': { | ||
| description: 'Opens an interactive web explorer for your CDK app', | ||
| options: { | ||
| port: { type: 'number', desc: 'Port to bind the web server on', default: 3411 }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🟡 Recommended — The explore command declares port with default: 3411, so yargs always populates args.port even when the user omits --port, and that concrete value is forwarded to startWebServer. In http-server.ts, a defined port sets startPort = endPort = port (no search, hard fail on conflict), while an undefined port searches DEFAULT_PORT..DEFAULT_PORT+100. Because the CLI always supplies 3411, the auto-increment fallback is unreachable from cdk explore: a user whose 3411 is already busy — e.g. a second cdk explore — gets a hard Port 3411 is already in use error instead of binding 3412.
Suggested change: Drop default: 3411 from the explore port option (keep only desc) so an omitted --port reaches startWebServer as undefined and the port search runs, while an explicit --port 3411 still fails hard. DEFAULT_PORT in http-server.ts already supplies 3411 as the search base, preserving the user-visible default.
|
|
||
| try { | ||
| const diskLocation = BUNDLED_FILES.includes(name) | ||
| ? path.resolve(__dirname, 'static', `${name}`) | ||
| : path.resolve(__dirname, '..', '..', 'frontend', 'src', `${name}`) | ||
| ; | ||
|
|
||
| return { | ||
| contentType: CONTENT_TYPES[name], |
There was a problem hiding this comment.
⚪ Optional — When CDK_LIVE is set, webAsset(name) resolves path.resolve(__dirname, '..', '..', 'frontend', 'src', name) with name taken straight from the /:asset route param, which the expressy matcher captures as ([^/]+) and then decodeURIComponents. A request like GET /..%2F..%2Fsecret yields name = '../../secret' and escapes the intended frontend/src directory. This is the one asset path that bypasses resolveWithinRoot, unlike the bundled-file allow-list and the /api/file and /api/template handlers that enforce containment. Reachable only in the developer-only CDK_LIVE mode, behind sessionAuth, and frontend/src is excluded from the published package, so there is no end-user exposure — but it is worth hardening for defense-in-depth and consistency.
Suggested change: Route the CDK_LIVE path through resolveWithinRoot(path.resolve(__dirname, '..', '..', 'frontend', 'src'), name) (and the static dir for bundled files) and return 404 when it returns undefined, mirroring /api/file and /api/template.
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| .command('explore', 'Opens an interactive web explorer for your CDK app', (yargs: Argv) => | ||
| yargs.option('port', { | ||
| default: 3411, | ||
| type: 'number', | ||
| desc: 'Port to bind the web server on', | ||
| }), | ||
| ) |
There was a problem hiding this comment.
🔴 Blocking — This generated parser declares the explore port option with default: 3411, but its source of truth cli-config.ts declares port: { type: 'number', desc: ... } with no default — so this file and user-input.ts (which carries @default - 3411) are out of sync with the generator and appear hand-edited. Generated arg files must never be edited by hand because the next regeneration silently overwrites them (AGENTS.md#projen). The next yarn projen run will rewrite default: 3411 to default: undefined, flipping observable behavior: cdk explore with no --port goes from hard-failing when 3411 is busy to auto-searching the next 100 ports.
Suggested change: Decide the intended default and encode it in cli-config.ts only, then regenerate so all three files are the output of a single generator run. For auto-search (as startWebServer's doc comment states), leave port with no default in cli-config.ts and regenerate so the generated files carry default: undefined.
| */ | ||
| export function createApp(): Express { | ||
| const router = createRouter(); | ||
|
|
||
| const app = ((raw: IncomingMessage, rawRes: ServerResponse): void => { | ||
| const res = makeResponse(rawRes); | ||
|
|
||
| // Parsing the request can fail, handle it gracefully | ||
| let req: Request; | ||
| try { | ||
| req = makeRequest(raw); | ||
| } catch (e) { | ||
| // eslint-disable-next-line no-console | ||
| console.error(`${raw.method} ${raw.url}:`, e); | ||
| res.status(400).end(); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| router.handle(req, res, req.path, () => { | ||
| // Nothing matched: mirror express's default 404. | ||
| if (!res.headersSent) { | ||
| res.status(404); | ||
| res.set('Content-Type', 'text/plain; charset=utf-8'); |
There was a problem hiding this comment.
🟡 Recommended — The request-parse and handler-dispatch failure paths in createApp write diagnostics with console.error behind // eslint-disable-next-line no-console directives, in library code bundled into the CLI. The guidelines say library output must go through the IoHost pattern rather than console, and eslint-disable directives must not be left in place (AGENTS.md#anti-patterns). The framework already threads an injected RequestLogger at the router level and server.ts wires a logger that routes to the IoHost, but this app-level error sink does not use it, so these diagnostics escape to raw stderr and are invisible to the IoHost/telemetry.
Suggested change: Let createApp accept an optional error-logging callback (defaulting to a no-op or to the same RequestLogger the server already owns) and call it instead of console.error, removing the eslint-disable directives.
Serve a local web UI that shows a synthesized app three ways -- construct tree, authored source, and CloudFormation template -- with navigation between them and policy-validation findings in context. - `cdk explore [--port]` starts a local-only server (default 3411) - `lib/web/`: routes for tree/template/file/policy-validation, SSE live refresh on re-synth, source staleness tracking, and local-only, security-header and session-token middleware - `lib/expressy/`: a small HTTP layer replacing Express, so the CLI bundle does not carry a web framework - `lib/web-shared/`: the wire protocol and severity model shared with the SPA - `frontend/`: React + Cloudscape single-page app, type-checked separately and bundled with esbuild into the published package
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| let startPort: number; | ||
| let endPort: number; | ||
| if (this.options.port) { | ||
| startPort = this.options.port; | ||
| endPort = startPort; | ||
| } else { | ||
| startPort = DEFAULT_PORT; | ||
| endPort = startPort + 100; |
There was a problem hiding this comment.
⚪ Optional — start() selects the port with if (this.options.port), a truthiness check. --port 0 is a legitimate value meaning 'let the OS assign a free ephemeral port', but 0 is falsy, so the code ignores it and instead auto-increments from DEFAULT_PORT (3411). A user who explicitly asks for an ephemeral port silently gets a 3411-range port instead.
Suggested change: Test for presence rather than truthiness, e.g. if (this.options.port !== undefined).
| * `synth.lock` appearing is instead reported as `onSynthActivity`, the only | ||
| * signal we get that a synth has started (the explorer never starts one itself). | ||
| */ |
There was a problem hiding this comment.
⚪ Optional — This doc comment says synth.lock appearing is reported as onSynthActivity, but the option added in the same diff is named onSynthStart (and it calls through to the synth-start staleness path). A comment the diff itself makes inaccurate misleads the next reader; the server.ts call-site comment similarly refers to onSynthActivity.
Suggested change: Update the comment here and the matching server.ts call-site comment to name onSynthStart.
|
Total lines changed 9261 is greater than 1000. Please consider breaking this PR down. |
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This branch had an error being deployed
4 participants
Serve a local web UI that shows the construct tree of a synthesized app, and maps between construct tree node, source view, and template view. The nodes and source lines are annotated with any diagnostics that the app may have produced.
cdk explore [--port]starts a local-only server (default port 3411).The application is a React + Cloudscape SPA, hitting a local backend server that services reads from the CloudAssembly. The user interface is organized into tabs, which can be extended in the future with different types of views (diagrams, ops actions, etc).
Closes #1748.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license