Open
Conversation
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
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 pull request restructures the charting libraries by extracting the financial chart extension into a dedicated library at Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Deploying stock-charts with
|
| Latest commit: |
3207602
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4a7cb132.stock-charts.pages.dev |
| Branch Preview URL: | https://reusable-charts.stock-charts.pages.dev |
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: DaveSkender <8432125+DaveSkender@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
…harts into reusable-charts
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: DaveSkender <8432125+DaveSkender@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Member
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
scripts/setup-linux.sh (1)
254-256:⚠️ Potential issue | 🟠 MajorAvoid piping the CodeRabbit installer directly into
bash.Line 255 still uses
curl ... | bash, which preserves a supply-chain execution risk and weakens forensic/debug visibility on installer failures.🔒 Proposed fix
- if curl -fsSL https://cli.coderabbit.ai/install.sh | bash; then # nosemgrep: bash.curl.security.curl-pipe-bash + local cr_install_script + cr_install_script=$(mktemp /tmp/coderabbit-install-XXXXXX.sh) + if curl -fsSL -o "$cr_install_script" https://cli.coderabbit.ai/install.sh && \ + [ -s "$cr_install_script" ] && \ + bash "$cr_install_script"; then log "CodeRabbit CLI installed" else warn "CodeRabbit CLI installation failed or skipped" fi + rm -f "$cr_install_script"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-linux.sh` around lines 254 - 256, Replace the unsafe piped-install pattern `curl -fsSL https://cli.coderabbit.ai/install.sh | bash` with a download-then-execute flow: use curl to save the installer to a temporary file, verify its integrity (checksum or signature) if available, make the file executable, run it explicitly with `bash <tempfile>`, and remove the temp file; update the log message ("CodeRabbit CLI installed") to only run after successful verification and execution. Ensure the change targets the exact occurrence of `curl -fsSL https://cli.coderabbit.ai/install.sh | bash` in the script.
🧹 Nitpick comments (3)
.gitignore (1)
93-97: Clarify.claudeintent with an explicit comment.Line 93 (
# .claude/) is a bit ambiguous next to selective ignores on Line 94. Consider adding a short note that only local overrides are ignored while shared config stays versioned.💡 Suggested comment tweak
-# .claude/ +# Keep shared Claude config in git; ignore only local machine overrides. .claude/*.local.json .codex/ .codacy/ .copilot/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 93 - 97, Replace the ambiguous comment before the .claude entries with a short explanatory note clarifying intent: state that only local override files (e.g., *.local.json) are ignored while shared/committed .claude configuration files remain versioned so readers understand why Line with ".claude/*.local.json" is excluded but other .claude files are kept.client/src/app/services/chart.service.spec.ts (1)
317-322: Remove unnecessary.pipe()call.The
.pipe()method without any operators is redundant and can be removed for cleaner code.♻️ Suggested simplification
const resizeObs = mockWindowService.getResizeObservable?.(); if (resizeObs) { - resizeObs.pipe().subscribe(dimensions => { + resizeObs.subscribe(dimensions => { service.onWindowResize(dimensions); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/services/chart.service.spec.ts` around lines 317 - 322, Remove the unnecessary .pipe() call when subscribing to the resize observable: call mockWindowService.getResizeObservable?.() to obtain resizeObs and subscribe directly (resizeObs.subscribe(...)) so the code uses resizeObs and service.onWindowResize(dimensions) without the redundant .pipe() invocation.scripts/setup-linux.sh (1)
307-308:main()has a redundant pre-call tosetup_nvm.
setup_node()already callssetup_nvmwhen Node is missing, so callingsetup_nvmagain inmain()adds duplicate work and extra failure surface with no functional gain.♻️ Proposed cleanup
- setup_nvm || warn "nvm setup had issues, continuing" setup_node || { err "Node.js setup failed"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-linux.sh` around lines 307 - 308, In main(), remove the redundant pre-call to setup_nvm (the line "setup_nvm || warn ...") because setup_node already invokes setup_nvm when Node is missing; leave the setup_node invocation and its failure handling intact (keep "setup_node || { err ...; exit 1; }") so Node installation and error reporting remain unchanged, and update any surrounding comments if needed to reflect that setup_nvm is now only called from setup_node.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vscode/tasks.json:
- Around line 19-23: The current tasks.json replaces the default cross-platform
setup task with a Linux-only task (label "Setup: Dev environment (linux)" that
runs "bash scripts/setup-linux.sh"); revert this by restoring a generic "Setup:
Dev environment" task that works for all contributors and move the
Linux-specific invocation into a platform-filtered task or a separate label —
update the task with a neutral command (e.g., call a unified setup script like
"scripts/setup.sh" that dispatches by OS) or add OS-specific tasks with "label"
values and "options.platform" (or "osx"/"windows"/"linux" conditionals) so
non-Linux users get the default one-setup-task workflow while keeping the Linux
script available as "Setup: Dev environment (linux)".
In `@client/package.json`:
- Around line 7-8: The `@types/node` devDependency must match the declared Node
runtime major (24) to avoid exposing Node 25 APIs; update package.json's
`@types/node` entry (in devDependencies) from ^25.3.0 to a 24.x range (for example
^24.0.0 or an exact 24.x version) so the TypeScript type definitions align with
the "node": ">=24 <25" engine constraint.
- Around line 57-58: The dev dependencies `@angular/build` and `@angular/cli` are at
21.1.5 while framework packages target 21.2.0; update the versions for
consistency by changing the entries "@angular/build" and "@angular/cli" in
package.json to "21.2.0" (or to match the exact framework versions used) and
then run your install command to regenerate lockfile and verify builds.
In `@docs/CONTRIBUTING.md`:
- Line 36: Replace the typo "cusomt" with "custom" in the sentence that
currently reads "unique configuration folder for cusomt agents or agent skills"
so the documentation reads "unique configuration folder for custom agents or
agent skills"; ensure the corrected word appears in the same sentence within
CONTRIBUTING.md and run the markdown lint/format checks to confirm no other MD
issues are introduced.
In `@scripts/setup-linux.sh`:
- Around line 125-129: The setup_dotnet function currently returns as soon as
any dotnet binary exists; change it to verify the required SDK version
DOTNET_VERSION (default 10.0) is installed by running dotnet --list-sdks and
checking for a line that begins with DOTNET_VERSION (or matches
"$DOTNET_VERSION" prefix); if the required SDK is present, log existing SDKs and
return 0, otherwise continue with the installation flow (do not return early).
Use the dotnet --list-sdks command output parsing in setup_dotnet to detect the
version and only skip install when the expected SDK is found.
---
Duplicate comments:
In `@scripts/setup-linux.sh`:
- Around line 254-256: Replace the unsafe piped-install pattern `curl -fsSL
https://cli.coderabbit.ai/install.sh | bash` with a download-then-execute flow:
use curl to save the installer to a temporary file, verify its integrity
(checksum or signature) if available, make the file executable, run it
explicitly with `bash <tempfile>`, and remove the temp file; update the log
message ("CodeRabbit CLI installed") to only run after successful verification
and execution. Ensure the change targets the exact occurrence of `curl -fsSL
https://cli.coderabbit.ai/install.sh | bash` in the script.
---
Nitpick comments:
In @.gitignore:
- Around line 93-97: Replace the ambiguous comment before the .claude entries
with a short explanatory note clarifying intent: state that only local override
files (e.g., *.local.json) are ignored while shared/committed .claude
configuration files remain versioned so readers understand why Line with
".claude/*.local.json" is excluded but other .claude files are kept.
In `@client/src/app/services/chart.service.spec.ts`:
- Around line 317-322: Remove the unnecessary .pipe() call when subscribing to
the resize observable: call mockWindowService.getResizeObservable?.() to obtain
resizeObs and subscribe directly (resizeObs.subscribe(...)) so the code uses
resizeObs and service.onWindowResize(dimensions) without the redundant .pipe()
invocation.
In `@scripts/setup-linux.sh`:
- Around line 307-308: In main(), remove the redundant pre-call to setup_nvm
(the line "setup_nvm || warn ...") because setup_node already invokes setup_nvm
when Node is missing; leave the setup_node invocation and its failure handling
intact (keep "setup_node || { err ...; exit 1; }") so Node installation and
error reporting remain unchanged, and update any surrounding comments if needed
to reflect that setup_nvm is now only called from setup_node.
ℹ️ Review info
Configuration used: Repository: facioquo/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.gitignore.vscode/tasks.jsonREADME.mdclient/package.jsonclient/src/app/services/chart.service.spec.tsdocs/CONTRIBUTING.mdlibs/chartjs-financial/controllers/candlestick.controller.tslibs/chartjs-financial/controllers/ohlc.controller.tslibs/indy-charts/charts/chart-manager.tspackage.jsonscripts/setup-linux.shscripts/start-functions.jsserver/Directory.Packages.propsserver/WebApi.Tests/packages.lock.jsontests/vitepress/.vitepress/theme/components/IndyIndicatorsDemo.vue
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/start-functions.js
- README.md
- libs/indy-charts/charts/chart-manager.ts
- server/Directory.Packages.props
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/plans/reusable-components/task-02.md`:
- Around line 81-85: Update the verification steps to target the new extracted
library workspace `libs/indy-charts` instead of `@stock-charts/client`: change
build command to run the library build for `libs/indy-charts` (e.g., the
equivalent pnpm build command for that workspace), run TypeScript compilation
and tests against `libs/indy-charts` (replace `pnpm test
--workspace=@stock-charts/client` with the test command for `libs/indy-charts`),
and confirm relevant spec files (e.g., ConfigService.spec.ts or its new
location/name under `libs/indy-charts`) still pass; additionally add a markdown
verification step for `**/*.md` to check for broken links and formatting per the
repo markdown configuration.
In `@docs/plans/reusable-components/task-04.md`:
- Around line 10-12: The doc mixes API method names causing inconsistency:
standardize all examples in this task to use one naming convention (prefer the
fetch* names shown: createApiClient, fetchQuotes, fetchIndicatorListings,
fetchIndicatorData) and update any occurrences of
getQuotes/getListings/getSelectionData to the corresponding fetch* variants;
also ensure references to ChartManager and its localStorage integration use the
same method names and update sections 71-76 and 88-91 accordingly, plus run a
quick markdown lint for broken links/formatting per the `**/*.md` guidelines.
- Around line 79-83: Update the checklist so verification steps do not exercise
APIs marked “not implemented”: change the entries for
ChartManager.enableCaching, ChartManager.restoreState, and LocalStorage caching
to “deferred / future validation” (or move them under a “Not yet implemented”
section) so they won’t be executed during verification, and ensure the note
about backward-compatible localStorage format remains consistent; also run the
markdown linter for docs/plans/reusable-components/task-04.md to fix any broken
links/formatting per the `**/*.md` guideline.
In `@docs/plans/reusable-components/task-05.md`:
- Around line 77-85: The checklist still references the old workspace name
`@stock-charts/client`; update the commands and verification steps to target the
current `@facioquo/*` packages (either the specific private package names or the
glob `@facioquo/*`). Replace `pnpm build:lib --workspace=@stock-charts/client`
and `pnpm pack --workspace=@stock-charts/client` with commands that run against
the correct workspace(s) (e.g., `pnpm build:lib --workspace=@facioquo/client` or
`pnpm build:lib --workspace=@facioquo/*`) and update the verification text to
assert dist/ contents for the `@facioquo/*` package(s) instead of
`@stock-charts/client`. Ensure any examples, package identifiers, and checklist
bullets consistently use `@facioquo/*` so the steps remain executable.
ℹ️ Review info
Configuration used: Repository: facioquo/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by**
📒 Files selected for processing (15)
.github/workflows/ci.yml.stylelintrc.json.vscode/tasks.jsondocs/plans/reusable-components/plan.mddocs/plans/reusable-components/task-01.mddocs/plans/reusable-components/task-02.mddocs/plans/reusable-components/task-03.mddocs/plans/reusable-components/task-04.mddocs/plans/reusable-components/task-05.mddocs/plans/reusable-components/task-06.mddocs/plans/reusable-components/task-07.mddocs/plans/reusable-components/task-08.mdpackage.jsonscripts/website-lint.shtests/vitepress/.stylelintrc.json
✅ Files skipped from review due to trivial changes (1)
- docs/plans/reusable-components/task-01.md
🚧 Files skipped from review as they are similar to previous changes (6)
- .github/workflows/ci.yml
- scripts/website-lint.sh
- docs/plans/reusable-components/task-07.md
- docs/plans/reusable-components/task-08.md
- docs/plans/reusable-components/task-03.md
- docs/plans/reusable-components/task-06.md
Comment on lines
+81
to
+85
| 1. Build library: `pnpm build:lib --workspace=@stock-charts/client` | ||
| 2. Verify TypeScript compilation succeeds | ||
| 3. Verify no Angular imports in extracted files | ||
| 4. Run existing tests: `pnpm test --workspace=@stock-charts/client` | ||
| 5. Verify ConfigService.spec.ts still passes |
Contributor
There was a problem hiding this comment.
Verification section is still anchored to legacy workspace flow.
This task says extraction moved to libs/indy-charts, but the validation steps still center on @stock-charts/client. Please update the commands/checks so they verify the current library workspace directly.
As per coding guidelines **/*.md: Check for broken links and formatting according to markdown configuration and instructions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/reusable-components/task-02.md` around lines 81 - 85, Update the
verification steps to target the new extracted library workspace
`libs/indy-charts` instead of `@stock-charts/client`: change build command to
run the library build for `libs/indy-charts` (e.g., the equivalent pnpm build
command for that workspace), run TypeScript compilation and tests against
`libs/indy-charts` (replace `pnpm test --workspace=@stock-charts/client` with
the test command for `libs/indy-charts`), and confirm relevant spec files (e.g.,
ConfigService.spec.ts or its new location/name under `libs/indy-charts`) still
pass; additionally add a markdown verification step for `**/*.md` to check for
broken links and formatting per the repo markdown configuration.
Comment on lines
+10
to
+12
| - Implement `createApiClient(baseUrl)` factory | ||
| - Implement fetch methods (fetchQuotes, fetchIndicatorListings, fetchIndicatorData) | ||
| - Add localStorage integration to ChartManager |
Contributor
There was a problem hiding this comment.
API method names are inconsistent within the same task doc.
This file mixes fetch* calls with getQuotes/getListings/getSelectionData. That reintroduces the API drift called out in PR feedback and makes copy/paste examples unreliable.
🛠️ Suggested doc fix
-- Implement fetch methods (fetchQuotes, fetchIndicatorListings, fetchIndicatorData)
+- Implement fetch methods (`getQuotes`, `getListings`, `getSelectionData`)
-const quotes = await client.fetchQuotes();
+const quotes = await client.getQuotes();As per coding guidelines **/*.md: Check for broken links and formatting according to markdown configuration and instructions.
Also applies to: 71-76, 88-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/reusable-components/task-04.md` around lines 10 - 12, The doc
mixes API method names causing inconsistency: standardize all examples in this
task to use one naming convention (prefer the fetch* names shown:
createApiClient, fetchQuotes, fetchIndicatorListings, fetchIndicatorData) and
update any occurrences of getQuotes/getListings/getSelectionData to the
corresponding fetch* variants; also ensure references to ChartManager and its
localStorage integration use the same method names and update sections 71-76 and
88-91 accordingly, plus run a quick markdown lint for broken links/formatting
per the `**/*.md` guidelines.
Comment on lines
+79
to
+83
| - [ ] `ChartManager.enableCaching(key)` — **not implemented** | ||
| - [ ] `ChartManager.restoreState()` — **not implemented** | ||
| - [ ] LocalStorage caching (SSR-safe) — **not implemented** | ||
| - [ ] Backward-compatible localStorage format — **not applicable (caching not built)** | ||
|
|
Contributor
There was a problem hiding this comment.
Verification steps currently exercise APIs explicitly marked “not implemented.”
The checklist invokes caching flows while this same section marks caching as not implemented. Mark these steps as deferred/future validation to avoid false failures and confusion.
As per coding guidelines **/*.md: Check for broken links and formatting according to markdown configuration and instructions.
Also applies to: 97-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/reusable-components/task-04.md` around lines 79 - 83, Update the
checklist so verification steps do not exercise APIs marked “not implemented”:
change the entries for ChartManager.enableCaching, ChartManager.restoreState,
and LocalStorage caching to “deferred / future validation” (or move them under a
“Not yet implemented” section) so they won’t be executed during verification,
and ensure the note about backward-compatible localStorage format remains
consistent; also run the markdown linter for
docs/plans/reusable-components/task-04.md to fix any broken links/formatting per
the `**/*.md` guideline.
Comment on lines
+77
to
+85
| 1. Run build: `pnpm build:lib --workspace=@stock-charts/client` | ||
| 2. Verify dist/ directory created with compiled files | ||
| 3. Check dist/ contains: | ||
| - JavaScript files (.js) | ||
| - TypeScript declarations (.d.ts) | ||
| - No test files (.spec.ts) | ||
|
|
||
| 4. Run `pnpm pack --workspace=@stock-charts/client` | ||
| 5. Extract .tgz and verify contents |
Contributor
There was a problem hiding this comment.
Verification steps still target obsolete workspace/package names.
The checklist still validates @stock-charts/client, but this task now documents two private @facioquo/* workspaces. These commands should be updated to keep the task executable as written.
🛠️ Suggested doc fix
-1. Run build: `pnpm build:lib --workspace=@stock-charts/client`
-2. Verify dist/ directory created with compiled files
+1. Build chartjs-financial: `pnpm --filter `@facioquo/chartjs-chart-financial` run build`
+2. Build indy-charts: `pnpm --filter `@facioquo/indy-charts` run build`
+3. Verify both `libs/chartjs-financial/dist/` and `libs/indy-charts/dist/` are created
-4. Run `pnpm pack --workspace=@stock-charts/client`
-5. Extract .tgz and verify contents
+4. Run `pnpm --filter `@facioquo/chartjs-chart-financial` pack`
+5. Run `pnpm --filter `@facioquo/indy-charts` pack`
+6. Extract `.tgz` files and verify contentsAs per coding guidelines **/*.md: Check for broken links and formatting according to markdown configuration and instructions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/reusable-components/task-05.md` around lines 77 - 85, The
checklist still references the old workspace name `@stock-charts/client`; update
the commands and verification steps to target the current `@facioquo/*` packages
(either the specific private package names or the glob `@facioquo/*`). Replace
`pnpm build:lib --workspace=@stock-charts/client` and `pnpm pack
--workspace=@stock-charts/client` with commands that run against the correct
workspace(s) (e.g., `pnpm build:lib --workspace=@facioquo/client` or `pnpm
build:lib --workspace=@facioquo/*`) and update the verification text to assert
dist/ contents for the `@facioquo/*` package(s) instead of
`@stock-charts/client`. Ensure any examples, package identifiers, and checklist
bullets consistently use `@facioquo/*` so the steps remain executable.
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
libs/chartjs-financial/eslint.config.spec.ts (1)
10-33: Consider extracting the repeatedConfigBlocktype.The
ConfigBlocktype is defined inline three times with slight variations. A single shared type at the top would reduce duplication.Suggested refactor
+type ConfigBlock = { ignores?: string[]; files?: string[] }; + describe("eslint.config", () => { it("exports a valid non-empty flat config array", () => { expect(Array.isArray(config)).toBe(true); expect((config as unknown[]).length).toBeGreaterThan(0); }); it("includes an ignores block for dist and node_modules", () => { - type ConfigBlock = { ignores?: string[]; files?: string[] }; const ignoresBlock = (config as ConfigBlock[]).find( c => "ignores" in c && !("files" in c) ); expect(ignoresBlock?.ignores).toContain("dist/**"); expect(ignoresBlock?.ignores).toContain("node_modules/**"); }); it("includes a TypeScript files configuration block", () => { - type ConfigBlock = { files?: string[] }; const tsBlock = (config as ConfigBlock[]).find(c => c.files?.includes("**/*.ts") ); expect(tsBlock).toBeDefined(); }); it("includes a spec files configuration block", () => { - type ConfigBlock = { files?: string[] }; const specBlock = (config as ConfigBlock[]).find(c => c.files?.includes("**/*.spec.ts") ); expect(specBlock).toBeDefined(); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/chartjs-financial/eslint.config.spec.ts` around lines 10 - 33, Extract the repeated inline type into a single shared type declaration (e.g., declare type ConfigBlock = { ignores?: string[]; files?: string[] };) at the top of the spec file and then reuse that type for the existing variables (ignoresBlock, tsBlock, specBlock) and their casts (config as ConfigBlock[]); update any redundant local type declarations so all three tests reference the single ConfigBlock type and remove the duplicate inline definitions.libs/indy-charts/tsconfig.json (1)
22-23: Consider tightening test-file exclusion.
"include": ["**/*.ts"]is broad; adding**/*.test.tstoexcludehelps avoid accidental emit of test files.Suggested diff
- "exclude": ["node_modules", "dist", "**/*.spec.ts", "eslint.config.ts"] + "exclude": ["node_modules", "dist", "**/*.spec.ts", "**/*.test.ts", "eslint.config.ts"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/indy-charts/tsconfig.json` around lines 22 - 23, The tsconfig.json currently includes all .ts files via "include": ["**/*.ts"] which can unintentionally pick up test files; update the "exclude" array (next to the existing "node_modules", "dist", "**/*.spec.ts", "eslint.config.ts") to also exclude pattern "**/*.test.ts" so test files are not emitted—locate the exclude entry in libs/indy-charts/tsconfig.json and add the pattern to that array..vscode/tasks.json (1)
197-213: Consider adding explicit shell override forbashcommands.These tasks invoke
bash scripts/dotnet-lint.shdirectly, which may fail on Windows systems wherebashisn't the default shell. The "Delete: Claude temporary files" task (lines 557-562) correctly uses an explicit shell override. Apply the same pattern here for consistency.Proposed fix for Lint: Backend .NET (check)
{ "label": "Lint: Backend .NET (check)", "detail": "Check .NET code formatting with dotnet format (Roslynator optional)", "type": "shell", "command": "bash scripts/dotnet-lint.sh check", "dependsOn": ["Build: Backend .NET (incremental)"], "dependsOrder": "sequence", "group": "test", - "problemMatcher": "$msCompile" + "problemMatcher": "$msCompile", + "options": { + "shell": { + "executable": "/bin/bash", + "args": ["-lc"] + } + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vscode/tasks.json around lines 197 - 213, The tasks labeled "Lint: Backend .NET (check)" and "Lint: Backend .NET (fix)" run "bash scripts/dotnet-lint.sh ..." directly which can fail on Windows; update both task objects to include the explicit shell override (matching the "Delete: Claude temporary files" pattern) by adding the options.shell block that sets the executable to "bash" and args to ["-lc"] so the commands run under bash cross-platform and remain consistent with existing tasks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vscode/tasks.json:
- Around line 19-23: The setup task labeled "Setup: Dev environment (linux)"
currently invokes only "bash scripts/setup-linux.sh" while README and
CONTRIBUTING.md reference scripts/setup-macos.sh, scripts/setup-windows.sh and
scripts/setup-environment.sh that do not exist; either add the missing platform
scripts and a cross-platform dispatcher (implement scripts/setup-macos.sh,
scripts/setup-windows.sh, and scripts/setup-environment.sh with the same install
steps as setup-linux.sh) and update the VSCode task to call the dispatcher, or
update README and CONTRIBUTING.md to state Linux-only support and adjust the
task label/command to accurately reflect "linux-only" (edit the task label and
"command" value referencing "bash scripts/setup-linux.sh" and the docs
referencing the scripts).
In `@tests/vitepress/package.json`:
- Around line 14-19: The tests/vitepress package.json declares date-fns@^4.1.0
which conflicts with client/package.json's pinned ^3.0.0 and may break
chartjs-adapter-date-fns@3.0.0; update tests/vitepress package.json to use
date-fns@^3.0.0 (matching client/package.json) and run install/build to verify
VitePress demo and adapter still work; ensure any package-lock or pnpm-lock file
is updated and mention the change in the PR so CI uses the aligned dependency.
---
Nitpick comments:
In @.vscode/tasks.json:
- Around line 197-213: The tasks labeled "Lint: Backend .NET (check)" and "Lint:
Backend .NET (fix)" run "bash scripts/dotnet-lint.sh ..." directly which can
fail on Windows; update both task objects to include the explicit shell override
(matching the "Delete: Claude temporary files" pattern) by adding the
options.shell block that sets the executable to "bash" and args to ["-lc"] so
the commands run under bash cross-platform and remain consistent with existing
tasks.
In `@libs/chartjs-financial/eslint.config.spec.ts`:
- Around line 10-33: Extract the repeated inline type into a single shared type
declaration (e.g., declare type ConfigBlock = { ignores?: string[]; files?:
string[] };) at the top of the spec file and then reuse that type for the
existing variables (ignoresBlock, tsBlock, specBlock) and their casts (config as
ConfigBlock[]); update any redundant local type declarations so all three tests
reference the single ConfigBlock type and remove the duplicate inline
definitions.
In `@libs/indy-charts/tsconfig.json`:
- Around line 22-23: The tsconfig.json currently includes all .ts files via
"include": ["**/*.ts"] which can unintentionally pick up test files; update the
"exclude" array (next to the existing "node_modules", "dist", "**/*.spec.ts",
"eslint.config.ts") to also exclude pattern "**/*.test.ts" so test files are not
emitted—locate the exclude entry in libs/indy-charts/tsconfig.json and add the
pattern to that array.
ℹ️ Review info
Configuration used: Repository: facioquo/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by**
📒 Files selected for processing (14)
.vscode/tasks.jsonclient/package.jsonlibs/chartjs-financial/eslint.config.spec.tslibs/chartjs-financial/eslint.config.tslibs/chartjs-financial/package.jsonlibs/chartjs-financial/tsconfig.jsonlibs/indy-charts/eslint.config.spec.tslibs/indy-charts/eslint.config.tslibs/indy-charts/package.jsonlibs/indy-charts/tsconfig.jsonpackage.jsonserver/Functions/UpdateQuotes.cstests/playwright/vitepress.spec.tstests/vitepress/package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/chartjs-financial/package.json
- tests/playwright/vitepress.spec.ts
- libs/indy-charts/eslint.config.spec.ts
- libs/indy-charts/package.json
Comment on lines
+19
to
+23
| "label": "Setup: Dev environment (linux)", | ||
| "detail": "Install all development prerequisites (Node, .NET, Azure Functions Core Tools, pnpm)", | ||
| "type": "shell", | ||
| "command": "bash scripts/setup-environment.sh", | ||
| "group": "none", | ||
| "problemMatcher": [] | ||
| "command": "bash scripts/setup-linux.sh", | ||
| "group": "none" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Looking for setup scripts ==="
fd -t f 'setup.*\.(sh|ps1|bat|cmd)' scripts/ 2>/dev/null || echo "No matching setup scripts found"
echo ""
echo "=== All files in scripts/ directory ==="
fd -t f . scripts/ 2>/dev/null | head -20
echo ""
echo "=== Searching for setup or platform tasks in tasks.json ==="
rg -A 3 '"label".*[Ss]etup|platform' .vscode/tasks.json | head -40Repository: facioquo/stock-charts
Length of output: 1444
🏁 Script executed:
#!/bin/bash
echo "=== Checking for README or contributing guidelines ==="
fd -t f '(README|CONTRIBUTING|SETUP|setup|install)' -i . --max-depth 2 | head -10
echo ""
echo "=== Searching for platform/OS mentions in README or docs ==="
rg -l '(Windows|macOS|Mac|Linux|platform|supported|Prerequisites)' . -t md --max-count 5
echo ""
echo "=== Checking full setup task block to see if there are conditionals ==="
rg -A 10 '"label".*Setup.*Dev environment' .vscode/tasks.jsonRepository: facioquo/stock-charts
Length of output: 1044
🏁 Script executed:
#!/bin/bash
echo "=== Checking README for platform/setup info ==="
rg -A 5 -B 5 '(Windows|macOS|Mac|Linux|Prerequisites|Setup|Development)' README.md | head -50
echo ""
echo "=== Checking CONTRIBUTING.md for platform/setup info ==="
rg -A 5 -B 5 '(Windows|macOS|Mac|Linux|Prerequisites|Setup|Development)' docs/CONTRIBUTING.md | head -50Repository: facioquo/stock-charts
Length of output: 4630
Setup task and documentation are inconsistent with actual implementation.
The setup task is labeled "Setup: Dev environment (linux)" and runs only bash scripts/setup-linux.sh. However, the README and CONTRIBUTING.md document platform-specific setup scripts for macOS (scripts/setup-macos.sh) and Windows (scripts/setup-windows.sh), plus a cross-platform dispatcher (scripts/setup-environment.sh). None of these documented scripts exist in the repository—only scripts/setup-linux.sh is present.
This breaks the documented setup workflow for Windows and macOS contributors. Either implement the missing setup scripts and the corresponding cross-platform task, or update the documentation to reflect that Linux-only setup is currently supported.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.vscode/tasks.json around lines 19 - 23, The setup task labeled "Setup: Dev
environment (linux)" currently invokes only "bash scripts/setup-linux.sh" while
README and CONTRIBUTING.md reference scripts/setup-macos.sh,
scripts/setup-windows.sh and scripts/setup-environment.sh that do not exist;
either add the missing platform scripts and a cross-platform dispatcher
(implement scripts/setup-macos.sh, scripts/setup-windows.sh, and
scripts/setup-environment.sh with the same install steps as setup-linux.sh) and
update the VSCode task to call the dispatcher, or update README and
CONTRIBUTING.md to state Linux-only support and adjust the task label/command to
accurately reflect "linux-only" (edit the task label and "command" value
referencing "bash scripts/setup-linux.sh" and the docs referencing the scripts).
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/vitepress/.vitepress/theme/custom.css (1)
114-128: Merge duplicated breakpoint blocks for maintainability.Both blocks target the same breakpoint and can be consolidated into one media query to reduce duplication.
♻️ Proposed refactor
-@media (width <= 880px) { - .indy-demo__canvas-wrap--overlay { - aspect-ratio: 2; - } -} - .indy-demo__canvas-wrap--oscillator { aspect-ratio: 10; } `@media` (width <= 880px) { + .indy-demo__canvas-wrap--overlay { + aspect-ratio: 2; + } .indy-demo__canvas-wrap--oscillator { aspect-ratio: 8; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/vitepress/.vitepress/theme/custom.css` around lines 114 - 128, Consolidate the two identical `@media` (width <= 880px) blocks into a single media query: keep the global base rule .indy-demo__canvas-wrap--oscillator { aspect-ratio: 10; } as-is, then create one `@media` (width <= 880px) block that contains both .indy-demo__canvas-wrap--overlay { aspect-ratio: 2; } and .indy-demo__canvas-wrap--oscillator { aspect-ratio: 8; } so the small-screen override for --oscillator and the rule for --overlay live together and duplication is removed..vscode/tasks.json (2)
230-243: Add a problem matcher for CSS lint tasks.
Lint: CSS (check)andLint: CSS (fix)don’t publish diagnostics to VS Code Problems, unlike your other lint tasks. Adding a stylelint matcher would improve editor feedback without changing behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vscode/tasks.json around lines 230 - 243, The CSS lint tasks "Lint: CSS (check)" and "Lint: CSS (fix)" don't publish diagnostics; add a stylelint problem matcher to each task so VS Code Problems receives diagnostics. Update the tasks with a "problemMatcher" property referencing a built-in or custom stylelint matcher (e.g., "$stylelint" or a workspace-defined "stylelint") on both tasks (labels "Lint: CSS (check)" and "Lint: CSS (fix)"), ensuring the matcher name matches whatever is defined in your extension or tasks.json "problemMatchers" section so behavior doesn't change beyond surfacing diagnostics.
281-306: Use workspace-managed execution for CodeRabbit CLI tasks.Lines 284, 291, 298, and 305 call
coderabbitdirectly without a workspace package manager, depending on global PATH installation. To improve reproducibility across contributors and CI environments, wrap these withpnpm exec coderabbit(or add CodeRabbit to package.json as a dependency and create an npm script).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.vscode/tasks.json around lines 281 - 306, The tasks labeled "Review: CodeRabbit CLI (plain)", "Review: CodeRabbit CLI (prompt-only)", "Review: CodeRabbit CLI (uncommitted, plain)", and "Review: CodeRabbit CLI (uncommitted, prompt-only)" call the CLI directly via the "command": "coderabbit" entry; change each to use workspace-managed execution by invoking the package manager (e.g., replace the command with "pnpm" and prepend "exec","coderabbit" to the "args" array) or alternatively add CodeRabbit as a dependency and point these tasks to an npm script; update the task "args" arrays accordingly (e.g., ["exec","coderabbit","--plain"] or ["exec","coderabbit","--prompt-only","--type","uncommitted"]) so the tasks run reproducibly across environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/vitepress/.vitepress/theme/custom.css`:
- Around line 27-139: The CSS selectors use BEM-style names (e.g.,
indy-demo__header, indy-demo__title, indy-demo__status) which conflict with the
project's kebab-case lint rule; either rename these selectors to kebab-case
equivalents (for example change indy-demo__header -> indy-demo-header,
indy-demo__title -> indy-demo-title, indy-demo__status -> indy-demo-status and
update all references in templates/components) or relax the selector naming rule
in the linter configuration to accept BEM (update the selector pattern to allow
double underscores/double hyphens or add an exception for the indy-demo
namespace) so Codacy no longer flags them.
---
Nitpick comments:
In @.vscode/tasks.json:
- Around line 230-243: The CSS lint tasks "Lint: CSS (check)" and "Lint: CSS
(fix)" don't publish diagnostics; add a stylelint problem matcher to each task
so VS Code Problems receives diagnostics. Update the tasks with a
"problemMatcher" property referencing a built-in or custom stylelint matcher
(e.g., "$stylelint" or a workspace-defined "stylelint") on both tasks (labels
"Lint: CSS (check)" and "Lint: CSS (fix)"), ensuring the matcher name matches
whatever is defined in your extension or tasks.json "problemMatchers" section so
behavior doesn't change beyond surfacing diagnostics.
- Around line 281-306: The tasks labeled "Review: CodeRabbit CLI (plain)",
"Review: CodeRabbit CLI (prompt-only)", "Review: CodeRabbit CLI (uncommitted,
plain)", and "Review: CodeRabbit CLI (uncommitted, prompt-only)" call the CLI
directly via the "command": "coderabbit" entry; change each to use
workspace-managed execution by invoking the package manager (e.g., replace the
command with "pnpm" and prepend "exec","coderabbit" to the "args" array) or
alternatively add CodeRabbit as a dependency and point these tasks to an npm
script; update the task "args" arrays accordingly (e.g.,
["exec","coderabbit","--plain"] or
["exec","coderabbit","--prompt-only","--type","uncommitted"]) so the tasks run
reproducibly across environments.
In `@tests/vitepress/.vitepress/theme/custom.css`:
- Around line 114-128: Consolidate the two identical `@media` (width <= 880px)
blocks into a single media query: keep the global base rule
.indy-demo__canvas-wrap--oscillator { aspect-ratio: 10; } as-is, then create one
`@media` (width <= 880px) block that contains both
.indy-demo__canvas-wrap--overlay { aspect-ratio: 2; } and
.indy-demo__canvas-wrap--oscillator { aspect-ratio: 8; } so the small-screen
override for --oscillator and the rule for --overlay live together and
duplication is removed.
ℹ️ Review info
Configuration used: Repository: facioquo/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.vscode/tasks.jsonclient/src/main.tsscripts/start-functions.jsserver/WebApi/appsettings.jsontests/playwright/playwright.config.tstests/playwright/vitepress.spec.tstests/vitepress/.vitepress/theme/custom.csstests/vitepress/package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/playwright/vitepress.spec.ts
- client/src/main.ts
- server/WebApi/appsettings.json
- tests/playwright/playwright.config.ts
- scripts/start-functions.js
Comment on lines
+27
to
+139
| .indy-demo__header { | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: space-between; | ||
| gap: 12px; | ||
| margin-bottom: 10px; | ||
| } | ||
|
|
||
| .indy-demo__title { | ||
| margin: 0; | ||
| font-size: 14px; | ||
| letter-spacing: 0.02em; | ||
| text-transform: uppercase; | ||
| color: var(--vp-c-text-2); | ||
| } | ||
|
|
||
| .indy-demo__hint { | ||
| margin: 0; | ||
| font-size: 12px; | ||
| color: var(--vp-c-text-3); | ||
| } | ||
|
|
||
| .indy-demo__status { | ||
| border-radius: 12px; | ||
| padding: 10px 12px; | ||
| font-size: 13px; | ||
| line-height: 1.4; | ||
| margin-bottom: 12px; | ||
| } | ||
|
|
||
| .indy-demo__status--loading { | ||
| border: 1px solid color-mix(in srgb, var(--vp-c-brand-1) 35%, var(--vp-c-divider)); | ||
| background: color-mix(in srgb, var(--vp-c-brand-1) 8%, var(--vp-c-bg-soft)); | ||
| color: var(--vp-c-text-1); | ||
| } | ||
|
|
||
| .indy-demo__status--error { | ||
| border: 1px solid color-mix(in srgb, #d97706 55%, var(--vp-c-divider)); | ||
| background: color-mix(in srgb, #f59e0b 16%, var(--vp-c-bg-soft)); | ||
| color: var(--vp-c-text-1); | ||
| } | ||
|
|
||
| .indy-demo__status-actions { | ||
| margin-top: 10px; | ||
| } | ||
|
|
||
| .indy-demo__retry { | ||
| border: 1px solid var(--vp-c-divider); | ||
| border-radius: 999px; | ||
| padding: 5px 12px; | ||
| font-size: 12px; | ||
| background: var(--vp-c-bg); | ||
| color: var(--vp-c-text-1); | ||
| cursor: pointer; | ||
| } | ||
|
|
||
| .indy-demo__retry:hover { | ||
| border-color: var(--vp-c-brand-1); | ||
| } | ||
|
|
||
| .indy-demo__panel { | ||
| border: 1px solid var(--indy-demo-border); | ||
| border-radius: 12px; | ||
| background: color-mix(in srgb, var(--vp-c-bg) 94%, transparent); | ||
| padding: 10px; | ||
| overflow: hidden; | ||
| min-width: 0; | ||
| } | ||
|
|
||
| .indy-demo__panel-title { | ||
| margin: 0 0 8px; | ||
| font-size: 12px; | ||
| color: var(--vp-c-text-2); | ||
| letter-spacing: 0.03em; | ||
| text-transform: uppercase; | ||
| } | ||
|
|
||
| .indy-demo__canvas-wrap { | ||
| position: relative; | ||
| width: 100%; | ||
| min-height: 120px; | ||
| } | ||
|
|
||
| .indy-demo__canvas-wrap--overlay { | ||
| aspect-ratio: 2.5; | ||
| } | ||
|
|
||
| @media (width <= 880px) { | ||
| .indy-demo__canvas-wrap--overlay { | ||
| aspect-ratio: 2; | ||
| } | ||
| } | ||
|
|
||
| .indy-demo__canvas-wrap--oscillator { | ||
| aspect-ratio: 10; | ||
| } | ||
|
|
||
| @media (width <= 880px) { | ||
| .indy-demo__canvas-wrap--oscillator { | ||
| aspect-ratio: 8; | ||
| } | ||
| } | ||
|
|
||
| .indy-demo__canvas { | ||
| display: block; | ||
| } | ||
|
|
||
| .indy-demo__stack { | ||
| display: grid; | ||
| gap: 12px; | ||
| } | ||
|
|
||
| .indy-demo__osc-grid { |
Contributor
There was a problem hiding this comment.
Resolve selector naming rule mismatch (kebab-case vs BEM).
Codacy is flagging selectors from Line 27 onward because classes like indy-demo__header are not kebab-case under the current rule. Please either rename classes to match the enforced convention or update the lint selector pattern to explicitly allow BEM, so this doesn’t remain an unresolved static-analysis item.
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[notice] 27-27: tests/vitepress/.vitepress/theme/custom.css#L27
(selector) => Expected class selector "${selector}" to be kebab-case
[notice] 35-35: tests/vitepress/.vitepress/theme/custom.css#L35
(selector) => Expected class selector "${selector}" to be kebab-case
[notice] 43-43: tests/vitepress/.vitepress/theme/custom.css#L43
(selector) => Expected class selector "${selector}" to be kebab-case
[notice] 49-49: tests/vitepress/.vitepress/theme/custom.css#L49
(selector) => Expected class selector "${selector}" to be kebab-case
[notice] 57-57: tests/vitepress/.vitepress/theme/custom.css#L57
(selector) => Expected class selector "${selector}" to be kebab-case
[notice] 63-63: tests/vitepress/.vitepress/theme/custom.css#L63
(selector) => Expected class selector "${selector}" to be kebab-case
[notice] 69-69: tests/vitepress/.vitepress/theme/custom.css#L69
(selector) => Expected class selector "${selector}" to be kebab-case
[notice] 73-73: tests/vitepress/.vitepress/theme/custom.css#L73
(selector) => Expected class selector "${selector}" to be kebab-case
[notice] 83-83: tests/vitepress/.vitepress/theme/custom.css#L83
(selector) => Expected class selector "${selector}" to be kebab-case
[notice] 87-87: tests/vitepress/.vitepress/theme/custom.css#L87
(selector) => Expected class selector "${selector}" to be kebab-case
[notice] 96-96: tests/vitepress/.vitepress/theme/custom.css#L96
(selector) => Expected class selector "${selector}" to be kebab-case
[notice] 104-104: tests/vitepress/.vitepress/theme/custom.css#L104
(selector) => Expected class selector "${selector}" to be kebab-case
[notice] 110-110: tests/vitepress/.vitepress/theme/custom.css#L110
(selector) => Expected class selector "${selector}" to be kebab-case
[notice] 120-120: tests/vitepress/.vitepress/theme/custom.css#L120
(selector) => Expected class selector "${selector}" to be kebab-case
[notice] 130-130: tests/vitepress/.vitepress/theme/custom.css#L130
(selector) => Expected class selector "${selector}" to be kebab-case
[notice] 134-134: tests/vitepress/.vitepress/theme/custom.css#L134
(selector) => Expected class selector "${selector}" to be kebab-case
[notice] 139-139: tests/vitepress/.vitepress/theme/custom.css#L139
(selector) => Expected class selector "${selector}" to be kebab-case
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/vitepress/.vitepress/theme/custom.css` around lines 27 - 139, The CSS
selectors use BEM-style names (e.g., indy-demo__header, indy-demo__title,
indy-demo__status) which conflict with the project's kebab-case lint rule;
either rename these selectors to kebab-case equivalents (for example change
indy-demo__header -> indy-demo-header, indy-demo__title -> indy-demo-title,
indy-demo__status -> indy-demo-status and update all references in
templates/components) or relax the selector naming rule in the linter
configuration to accept BEM (update the selector pattern to allow double
underscores/double hyphens or add an exception for the indy-demo namespace) so
Codacy no longer flags them.
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
.devcontainer/devcontainer.json (1)
16-21: Consider dropping Zsh setup if Bash stays the default shell.Lines 16-17 install Zsh/Oh My Zsh, but Line 50 keeps Bash as default. If the team standard is Bash, removing Zsh setup can reduce container setup time.
Also applies to: 50-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.devcontainer/devcontainer.json around lines 16 - 21, Remove the unnecessary Zsh/Oh My Zsh installation by deleting or setting false the installZsh and installOhMyZsh entries in the devcontainer JSON (keys: "installZsh", "installOhMyZsh"), or alternatively set the default shell to Zsh if you intend to keep those installs; ensure the final devcontainer.json aligns with the team standard (Bash) so the container doesn't install Zsh/Oh My Zsh unnecessarily and thus reduces setup time.tests/playwright/website.spec.ts (1)
100-101: Consider replacing hardcoded timeout with a polling condition.The
waitForTimeout(2000)is a fixed delay that could cause flakiness — tests may fail on slower systems or wait unnecessarily on faster ones. Consider polling for the painted content condition instead.♻️ Suggested refactor using polling
- // Wait a bit for Chart.js rendering to complete - await page.waitForTimeout(2000); - - // Check if the canvas has non-blank content by evaluating pixel data - const hasContent = await page.evaluate(() => { + // Poll until canvas has painted content (Chart.js rendering complete) + await expect(async () => { + const hasContent = await page.evaluate(() => { const canvas = document.getElementById("chartOverlay") as HTMLCanvasElement | null; if (!canvas) return false; const ctx = canvas.getContext("2d"); if (!ctx) return false; const imageData = ctx.getImageData(0, 0, canvas.width, canvas.height); - // Check if any pixel has non-zero alpha (meaning something was drawn) for (let i = 3; i < imageData.data.length; i += 4) { if (imageData.data[i] > 0) return true; } return false; }); - - expect(hasContent, "Chart canvas should have painted content (not blank)").toBe(true); + expect(hasContent).toBe(true); + }).toPass({ timeout: 10_000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/playwright/website.spec.ts` around lines 100 - 101, Replace the brittle fixed delay await page.waitForTimeout(2000) in tests/playwright/website.spec.ts with a polling-based wait that checks for the chart being actually rendered (for example using page.waitForSelector for a chart element or page.waitForFunction that inspects the canvas/DOM or pixel data). Update the test to call page.waitForFunction or page.waitForSelector (targeting the Chart.js canvas, a legend, or a specific DOM node/text that appears only after rendering) so the test proceeds as soon as the chart is ready and avoids arbitrary sleeps.libs/indy-charts/helpers/apply-selection-tokens.spec.ts (1)
70-85: Tests for null and undefined appear redundant.Both test cases ("skips params with null value" and "skips params with undefined value") set
valuetoundefinedand assert identical behavior. Consider consolidating into a single parameterized test or ensuring one actually testsnull.♻️ Suggested fix for Line 72 to test null
it("skips params with null value", () => { const selection = makeSelection(); - selection.params[0].value = undefined; + selection.params[0].value = null as unknown as number; applySelectionTokens(selection); expect(selection.label).toBe("Bollinger Bands([P1],2)"); expect(selection.results[0].label).toBe("Upper([P1],2)"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/indy-charts/helpers/apply-selection-tokens.spec.ts` around lines 70 - 85, The two tests are redundant because both set selection.params[0].value to undefined; update the second spec so it actually tests null (or merge into a single parameterized test). Specifically, in the test that targets "skips params with null value", set selection.params[0].value = null (using makeSelection() and applySelectionTokens(selection)) and keep the same assertions on selection.label and selection.results[0].label; alternatively convert both cases into a table-driven test iterating values [null, undefined] and asserting identical behavior for applySelectionTokens.client/src/app/services/api.service.ts (1)
7-13: Consider using type-only import.All five imported symbols (
IndicatorListing,IndicatorParam,IndicatorSelection,Quote,RawQuote) are used only as type annotations in this file. Usingimport typewould make this explicit and ensure no runtime import overhead.♻️ Suggested change
-import { +import type { IndicatorListing, IndicatorParam, IndicatorSelection, Quote, RawQuote } from "@facioquo/indy-charts";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/services/api.service.ts` around lines 7 - 13, The five symbols IndicatorListing, IndicatorParam, IndicatorSelection, Quote, and RawQuote are only used as types so change the import to a type-only import by replacing the current import statement that pulls them from "@facioquo/indy-charts" with an "import type { IndicatorListing, IndicatorParam, IndicatorSelection, Quote, RawQuote } from '@facioquo/indy-charts';" to avoid runtime import overhead; verify there are no runtime references to those identifiers elsewhere in the file and run TypeScript build to ensure no errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vscode/extensions.json:
- Line 19: Add the main Copilot extension entry next to the existing
"GitHub.copilot-chat" item in the VS Code recommended extensions list: include
"GitHub.copilot" alongside "GitHub.copilot-chat" in the extensions array so both
"GitHub.copilot-chat" and "GitHub.copilot" are recommended.
In `@client/src/app/services/chart.service.ts`:
- Around line 236-253: Wrap localStorage interactions in cacheSelections and
loadSelections with try/catch to prevent storage or parsing errors from aborting
chart flows: in cacheSelections (method cacheSelections) catch errors from
JSON.stringify and localStorage.setItem and log/report them (do not throw), and
in loadSelections (method loadSelections) catch errors from localStorage.getItem
and JSON.parse, call loadDefaultSelections and optionally clear the corrupt
"selections" key or fallback to an empty array, then proceed to
addSelectionWithoutScroll for valid entries; ensure any caught errors are
handled gracefully and do not rethrow.
In `@libs/indy-charts/helpers/apply-selection-tokens.ts`:
- Around line 19-22: The current use of String.prototype.replace on
selection.label and result.label only replaces the first occurrence of token;
update the replacement calls in the apply-selection-tokens logic to use
replaceAll so every instance of token in selection.label and in each
result.label is substituted with valueText (refer to selection.label,
selection.results.forEach(result => ...), result.label, token and valueText to
locate the lines to change).
In `@libs/indy-charts/helpers/calculate-optimal-bars.ts`:
- Around line 25-26: Validate inputs before computing `calculated`: ensure
`pixelsPerBar` is a finite number > 0 and `containerWidth` is a finite number >=
0 (or choose a sensible fallback); if either check fails return `MIN_BARS` (or
another defined safe default) instead of performing Math.floor, then proceed to
compute `calculated = Math.floor(containerWidth / pixelsPerBar)` and clamp using
`Math.max(MIN_BARS, Math.min(MAX_BARS, calculated))`. Ensure checks are added in
the `calculateOptimalBars`/helper that uses `pixelsPerBar`, `containerWidth`,
`MIN_BARS`, and `MAX_BARS`.
In `@tests/playwright/website.spec.ts`:
- Around line 146-172: The test "no critical console errors during full page
lifecycle" collects consoleErrors via collectErrors(page), filters them into
criticalErrors, but never asserts they are empty; add an assertion to fail the
test when criticalErrors exist—e.g., use expect(criticalErrors, "No critical
console errors").toEqual([]) or expect(criticalErrors).toHaveLength(0). Place
this assertion after the filtering (and optionally before or after the existing
logging) to ensure the test fails on any non-whitelisted console errors.
---
Nitpick comments:
In @.devcontainer/devcontainer.json:
- Around line 16-21: Remove the unnecessary Zsh/Oh My Zsh installation by
deleting or setting false the installZsh and installOhMyZsh entries in the
devcontainer JSON (keys: "installZsh", "installOhMyZsh"), or alternatively set
the default shell to Zsh if you intend to keep those installs; ensure the final
devcontainer.json aligns with the team standard (Bash) so the container doesn't
install Zsh/Oh My Zsh unnecessarily and thus reduces setup time.
In `@client/src/app/services/api.service.ts`:
- Around line 7-13: The five symbols IndicatorListing, IndicatorParam,
IndicatorSelection, Quote, and RawQuote are only used as types so change the
import to a type-only import by replacing the current import statement that
pulls them from "@facioquo/indy-charts" with an "import type { IndicatorListing,
IndicatorParam, IndicatorSelection, Quote, RawQuote } from
'@facioquo/indy-charts';" to avoid runtime import overhead; verify there are no
runtime references to those identifiers elsewhere in the file and run TypeScript
build to ensure no errors.
In `@libs/indy-charts/helpers/apply-selection-tokens.spec.ts`:
- Around line 70-85: The two tests are redundant because both set
selection.params[0].value to undefined; update the second spec so it actually
tests null (or merge into a single parameterized test). Specifically, in the
test that targets "skips params with null value", set selection.params[0].value
= null (using makeSelection() and applySelectionTokens(selection)) and keep the
same assertions on selection.label and selection.results[0].label; alternatively
convert both cases into a table-driven test iterating values [null, undefined]
and asserting identical behavior for applySelectionTokens.
In `@tests/playwright/website.spec.ts`:
- Around line 100-101: Replace the brittle fixed delay await
page.waitForTimeout(2000) in tests/playwright/website.spec.ts with a
polling-based wait that checks for the chart being actually rendered (for
example using page.waitForSelector for a chart element or page.waitForFunction
that inspects the canvas/DOM or pixel data). Update the test to call
page.waitForFunction or page.waitForSelector (targeting the Chart.js canvas, a
legend, or a specific DOM node/text that appears only after rendering) so the
test proceeds as soon as the chart is ready and avoids arbitrary sleeps.
ℹ️ Review info
Configuration used: Repository: facioquo/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by**
📒 Files selected for processing (31)
.devcontainer/devcontainer.json.github/workflows/ci.yml.vscode/extensions.jsonclient/package.jsonclient/src/app/components/picker/pick-config.component.tsclient/src/app/components/picker/settings.component.tsclient/src/app/data/backup-quotes.tsclient/src/app/services/api.service.spec.tsclient/src/app/services/api.service.tsclient/src/app/services/chart.service.spec.tsclient/src/app/services/chart.service.tsclient/src/app/services/config.service.spec.tsclient/src/app/services/config.service.tsclient/src/app/services/user.service.tsclient/src/app/types/chart.types.tsclient/src/main.tsclient/src/test-utils/chart-test-harness.tsdocs/plans/reusable-charts.plan.mdlibs/indy-charts/README.mdlibs/indy-charts/config/index.tslibs/indy-charts/helpers/apply-selection-tokens.spec.tslibs/indy-charts/helpers/apply-selection-tokens.tslibs/indy-charts/helpers/calculate-optimal-bars.spec.tslibs/indy-charts/helpers/calculate-optimal-bars.tslibs/indy-charts/helpers/create-default-selection.spec.tslibs/indy-charts/helpers/create-default-selection.tslibs/indy-charts/helpers/index.tslibs/indy-charts/index.tslibs/indy-charts/package.jsontests/playwright/website.spec.tstests/vitepress/.vitepress/theme/components/indy-demo-utils.ts
💤 Files with no reviewable changes (2)
- client/src/app/services/config.service.ts
- client/src/app/services/config.service.spec.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- libs/indy-charts/package.json
- .github/workflows/ci.yml
- client/src/main.ts
- libs/indy-charts/config/index.ts
- libs/indy-charts/README.md
- docs/plans/reusable-charts.plan.md
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
libs/indy-charts/charts/chart-manager.spec.ts (3)
290-295: Test doesn't verify extraBars as stated in comment.The comment claims extraBars is "verified indirectly via render call count," but the test only asserts on
settings. Consider either updating the comment to reflect what's actually tested, or adding assertions that verify extraBars has an effect (e.g., checking render was called with the custom extraBars value).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/indy-charts/charts/chart-manager.spec.ts` around lines 290 - 295, The test for ChartManager incorrectly claims extraBars is verified but only asserts custom.settings; update the test to actually verify extraBars behavior: instantiate new ChartManager({ settings: defaultSettings, extraBars: 10 }), spy/mock the ChartManager.prototype.render (or the instance render method) to capture calls, trigger whatever initialization calls render, and assert that render was invoked with parameters or internal state reflecting extraBars === 10 (or that the render call count/args change as expected), then call custom.destroy(); alternatively, change the test comment to remove the misleading claim and only assert settings on the custom instance while keeping the existing assertions.
244-251: Consider using deterministic values for RSI in test fixtures.Using
Math.random()in test data can lead to non-reproducible test runs and makes debugging failures harder. While current tests don't assert on specific RSI values, deterministic data is a testing best practice.♻️ Suggested fix for deterministic test data
function makeIndicatorData(quotes: Quote[]): IndicatorDataRow[] { - return quotes.map(q => ({ + return quotes.map((q, i) => ({ date: q.date.toISOString(), candle: q, sma: q.close * 0.99, - rsi: 50 + Math.random() * 10 + rsi: 50 + (i % 10) })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/indy-charts/charts/chart-manager.spec.ts` around lines 244 - 251, The test fixture makeIndicatorData uses Math.random() for the rsi field which makes tests non-deterministic; change makeIndicatorData to use a deterministic value (for example derive rsi from the map index or from a stable property of Quote) by switching to quotes.map((q, i) => ...) and computing rsi deterministically (e.g. 50 + (i % 10) or another stable formula) so IndicatorDataRow.rsi is reproducible across runs.
363-375: Accessing private member_allProcessedDatasetscouples test to implementation.The test verifies internal state via
(mgr as any)._allProcessedDatasets.has("sel-2"). While sometimes necessary, this creates fragile tests that break when implementation details change. The comment notes setBarCount tests verify this indirectly—consider whether this explicit check adds sufficient value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/indy-charts/charts/chart-manager.spec.ts` around lines 363 - 375, Replace the fragile internal-state assertion that accesses (mgr as any)._allProcessedDatasets with a behavior-based check: after calling mgr.processSelectionData(selection, listing, makeIndicatorData(quotes)), mutate the original quotes dataset and then call the public API that relies on the stored copy (e.g., mgr.setBarCount or another public method used in slicing tests) and assert it still produces the expected result or does not throw; reference mgr.processSelectionData and mgr.setBarCount (and helpers makeOverlayListing/makeSelection) to locate where to change the test and remove the direct access to _allProcessedDatasets.tests/playwright/website.spec.ts (1)
101-101: Replace fixed sleeps with deterministic waits.Lines 101 and 154 use
waitForTimeout()with static delays (2000ms and 5000ms respectively), which are slower and more prone to flakiness than observable page conditions. Both of these sleeps occur after already waiting for element visibility and page load state—they likely can be removed or replaced with condition-based waits usingexpect.poll()orwaitForFunction()to detect actual readiness (e.g., canvas content availability or application state completion).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/playwright/website.spec.ts` at line 101, Replace the fixed sleeps (await page.waitForTimeout(2000) and the 5000ms sleep later) with deterministic waits: remove these waitForTimeout calls and instead use condition-based waits such as expect.poll(...) or page.waitForFunction(...) to watch for the actual readiness (for example, check that the target canvas has non-empty pixel data, a specific DOM element or class appears, or a JS application-ready flag becomes true); update the test around those calls (in tests/playwright/website.spec.ts) to wait for the observable condition after page.waitForSelector(...) or page.waitForLoadState(...) rather than sleeping so the test is faster and less flaky.client/src/app/services/user.service.ts (1)
3-3: Useimport typeforUserSettingsto align with TypeScript best practices.At Line 3,
UserSettingsis used only as a type annotation (line 9). Since it's imported from a type-only re-export inchart.types.ts, explicitly usingimport typeclarifies intent and follows modern TypeScript conventions.♻️ Proposed fix
-import { UserSettings } from "../types/chart.types"; +import type { UserSettings } from "../types/chart.types";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/services/user.service.ts` at line 3, The import of UserSettings should be a type-only import; update the import statement that currently reads importing UserSettings from "../types/chart.types" to use TypeScript's type-only form so the symbol UserSettings is imported with "import type" (this clarifies intent and avoids runtime import overhead where UserSettings is used as a type annotation in this module, e.g., in functions or variables referencing UserSettings in user.service.ts)..github/workflows/ci.yml (1)
14-14: Consider pinning Node to a full patch version (e.g.,24.11.0) across all workflows for deterministic CI.Using major-only versions like
24causes drift as new patches release. Whilepackage.jsonconstraints specify>=24 <25, the CI workflows all useNODE_VERSION: 24without patch-level pinning, meaning different CI runs could resolve to different patch versions. This impacts reproducibility across environments.This pattern is consistent across
.github/workflows/ci.yml,publish-packages.yml,deploy-website.yml, andcopilot-setup-steps.yml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml at line 14, The workflow uses a major-only Node version variable NODE_VERSION: 24 which can drift; update NODE_VERSION to a full patch pin (e.g., set NODE_VERSION to a specific patch like 24.11.0) in this CI workflow and make the same change consistently in the other workflow files (publish-packages.yml, deploy-website.yml, copilot-setup-steps.yml) so all uses of NODE_VERSION reference the identical exact patch version; search for the NODE_VERSION symbol in the workflows and replace the major-only value with the chosen full patch version.libs/indy-charts/helpers/apply-selection-tokens.spec.ts (1)
70-85: Test description mismatch: both tests verifyundefined, notnull.Line 72 sets
value = undefineddespite the test being named "skips params with null value". This makes both tests (lines 70-77 and 79-85) functionally identical, verifyingundefinedbehavior only. Consider either:
- Renaming line 70's test to accurately describe
undefinedhandling, or- Actually testing
nullin one of these tests if the implementation supports it♻️ Suggested fix
it("skips params with null value", () => { const selection = makeSelection(); - selection.params[0].value = undefined; + selection.params[0].value = null as unknown as number; applySelectionTokens(selection); expect(selection.label).toBe("Bollinger Bands([P1],2)"); expect(selection.results[0].label).toBe("Upper([P1],2)"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/indy-charts/helpers/apply-selection-tokens.spec.ts` around lines 70 - 85, The test named "skips params with null value" is actually assigning undefined, duplicating the "undefined" test; update the spec so one test truly covers null handling: in the test that uses makeSelection() and selection.params[0].value set on lines using applySelectionTokens, either change the test name to "skips params with undefined value" or (preferably) set selection.params[0].value = null and keep the original name, then run applySelectionTokens(selection) and keep the same expects; ensure you reference the same helpers (makeSelection and applySelectionTokens) and adjust the setup only for selection.params[0].value so the two tests exercise null vs undefined distinctly.libs/chartjs-financial/package.json (1)
19-21: Remove the upper-bound constraint from the Node engine specification.The
>=24 <25range will block installs on Node 25+, even if the package remains compatible. For published libraries, omitting upper bounds is npm best practice unless there's a known incompatibility. Since this package contains no native code and has no version conflicts with Node 25, use"node": ">=24"instead.Suggested change
"engines": { - "node": ">=24 <25" + "node": ">=24" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/chartjs-financial/package.json` around lines 19 - 21, The engines field in package.json currently restricts Node to ">=24 <25" which blocks Node 25+; update the "engines" entry so Node has no upper bound (change the "node" value to ">=24") in libs/chartjs-financial's package.json (look for the "engines" object and the "node" key) and commit the modified package.json.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/publish-packages.yml:
- Around line 3-6: The publish steps currently call `pnpm publish` directly (the
`pnpm publish` invocations) which will set the `latest` tag by default and will
fail if the same version is already published on workflow reruns; update the
workflow to detect prereleases (e.g., if the package version contains a
prerelease identifier like `-alpha`/`-rc`) and set `--tag next` for prereleases
and `--tag latest` for stable releases, and before running `pnpm publish` check
whether the target package@version already exists in the registry (skip publish
if it does) so reruns are safe; modify the steps that run `pnpm publish` to
compute the tag, perform a registry existence check (using npm/pnpm view or
registry API) and only run `pnpm publish --tag <computed-tag>` when the version
is not present.
In `@client/src/app/services/chart.service.spec.ts`:
- Around line 270-282: The teardown is double-destroying ChartManager:
getChartManager().destroy() is called and then service.ngOnDestroy() (which also
destroys the manager); fix by keeping a single destroy path—either remove the
explicit getChartManager().destroy() call and rely on service.ngOnDestroy(), or
make ChartManager.destroy()/service.ngOnDestroy() idempotent; update the test to
only call resizeSubject.complete() and then service.ngOnDestroy() (or conversely
only call getChartManager().destroy() and avoid calling service.ngOnDestroy())
so the manager is destroyed exactly once.
In `@client/src/app/services/chart.service.ts`:
- Around line 107-111: addSelectionWithoutScroll currently calls
this.addSelection(...).subscribe() with no error handler, which lets errors
rethrown from addSelection propagate as unhandled; update
addSelectionWithoutScroll to handle errors from addSelection by subscribing with
an error callback (or using subscribe({ next: ..., error: err => ... })) and log
or silently ignore the error so startup hydration failures don't surface as
unhandled exceptions; reference the addSelectionWithoutScroll method, the
addSelection(IndicatorSelection, listing, boolean) call, and the listings.find(x
=> x.uiid === selection.uiid) lookup when making the change.
In `@scripts/setup-linux.sh`:
- Around line 177-185: The script currently checks only for any dotnet binary
after running apt_install "dotnet-sdk-${dotnet_version}", which can hide
failures if an older SDK already exists; update the post-install verification to
parse dotnet --list-sdks and confirm that the requested dotnet_version (variable
dotnet_version) appears in the output (e.g., match a line starting with
"${dotnet_version}" or otherwise compare installed versions), and if it is not
found log an error (err "Failed to install .NET SDK ${dotnet_version}: installed
versions: ...") and return 1; keep the existing listing (dotnet --list-sdks) for
diagnostics but gate success on finding the specific requested version.
In `@tests/playwright/website.spec.ts`:
- Around line 66-88: The test "chart loads with correct bar count logged"
currently only checks console logs but misses asserting there were no uncaught
page errors; mirror the other tests by adding the page error assertion: ensure a
pageErrors collection (captured via page.on('pageerror', ...)) is asserted to be
empty (e.g., expect(pageErrors).toEqual([]) or equivalent) before finishing the
test so any uncaught runtime errors fail this test; update the test to reference
the pageErrors variable and place the assertion after page.waitForLoadState /
visibility check and before the final expectations.
- Line 3: The tests hardcode WEBSITE_URL ("WEBSITE_URL" constant) to
http://localhost:4200 and call page.goto(...) with that value; replace that
pattern by removing or updating the WEBSITE_URL constant and changing all
page.goto(...) calls that use it to page.goto("/") (or page.goto("/some/path")
for specific routes) so Playwright uses the configured baseURL from
playwright.config.ts; update any assertions that build URLs from WEBSITE_URL to
use relative paths as well.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 14: The workflow uses a major-only Node version variable NODE_VERSION: 24
which can drift; update NODE_VERSION to a full patch pin (e.g., set NODE_VERSION
to a specific patch like 24.11.0) in this CI workflow and make the same change
consistently in the other workflow files (publish-packages.yml,
deploy-website.yml, copilot-setup-steps.yml) so all uses of NODE_VERSION
reference the identical exact patch version; search for the NODE_VERSION symbol
in the workflows and replace the major-only value with the chosen full patch
version.
In `@client/src/app/services/user.service.ts`:
- Line 3: The import of UserSettings should be a type-only import; update the
import statement that currently reads importing UserSettings from
"../types/chart.types" to use TypeScript's type-only form so the symbol
UserSettings is imported with "import type" (this clarifies intent and avoids
runtime import overhead where UserSettings is used as a type annotation in this
module, e.g., in functions or variables referencing UserSettings in
user.service.ts).
In `@libs/chartjs-financial/package.json`:
- Around line 19-21: The engines field in package.json currently restricts Node
to ">=24 <25" which blocks Node 25+; update the "engines" entry so Node has no
upper bound (change the "node" value to ">=24") in libs/chartjs-financial's
package.json (look for the "engines" object and the "node" key) and commit the
modified package.json.
In `@libs/indy-charts/charts/chart-manager.spec.ts`:
- Around line 290-295: The test for ChartManager incorrectly claims extraBars is
verified but only asserts custom.settings; update the test to actually verify
extraBars behavior: instantiate new ChartManager({ settings: defaultSettings,
extraBars: 10 }), spy/mock the ChartManager.prototype.render (or the instance
render method) to capture calls, trigger whatever initialization calls render,
and assert that render was invoked with parameters or internal state reflecting
extraBars === 10 (or that the render call count/args change as expected), then
call custom.destroy(); alternatively, change the test comment to remove the
misleading claim and only assert settings on the custom instance while keeping
the existing assertions.
- Around line 244-251: The test fixture makeIndicatorData uses Math.random() for
the rsi field which makes tests non-deterministic; change makeIndicatorData to
use a deterministic value (for example derive rsi from the map index or from a
stable property of Quote) by switching to quotes.map((q, i) => ...) and
computing rsi deterministically (e.g. 50 + (i % 10) or another stable formula)
so IndicatorDataRow.rsi is reproducible across runs.
- Around line 363-375: Replace the fragile internal-state assertion that
accesses (mgr as any)._allProcessedDatasets with a behavior-based check: after
calling mgr.processSelectionData(selection, listing, makeIndicatorData(quotes)),
mutate the original quotes dataset and then call the public API that relies on
the stored copy (e.g., mgr.setBarCount or another public method used in slicing
tests) and assert it still produces the expected result or does not throw;
reference mgr.processSelectionData and mgr.setBarCount (and helpers
makeOverlayListing/makeSelection) to locate where to change the test and remove
the direct access to _allProcessedDatasets.
In `@libs/indy-charts/helpers/apply-selection-tokens.spec.ts`:
- Around line 70-85: The test named "skips params with null value" is actually
assigning undefined, duplicating the "undefined" test; update the spec so one
test truly covers null handling: in the test that uses makeSelection() and
selection.params[0].value set on lines using applySelectionTokens, either change
the test name to "skips params with undefined value" or (preferably) set
selection.params[0].value = null and keep the original name, then run
applySelectionTokens(selection) and keep the same expects; ensure you reference
the same helpers (makeSelection and applySelectionTokens) and adjust the setup
only for selection.params[0].value so the two tests exercise null vs undefined
distinctly.
In `@tests/playwright/website.spec.ts`:
- Line 101: Replace the fixed sleeps (await page.waitForTimeout(2000) and the
5000ms sleep later) with deterministic waits: remove these waitForTimeout calls
and instead use condition-based waits such as expect.poll(...) or
page.waitForFunction(...) to watch for the actual readiness (for example, check
that the target canvas has non-empty pixel data, a specific DOM element or class
appears, or a JS application-ready flag becomes true); update the test around
those calls (in tests/playwright/website.spec.ts) to wait for the observable
condition after page.waitForSelector(...) or page.waitForLoadState(...) rather
than sleeping so the test is faster and less flaky.
ℹ️ Review info
Configuration used: Repository: facioquo/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by**
📒 Files selected for processing (41)
.devcontainer/devcontainer.json.github/workflows/ci.yml.github/workflows/publish-packages.yml.npmrc.vscode/extensions.jsonclient/package.jsonclient/src/app/components/picker/pick-config.component.tsclient/src/app/components/picker/settings.component.tsclient/src/app/data/backup-quotes.tsclient/src/app/services/api.service.spec.tsclient/src/app/services/api.service.tsclient/src/app/services/chart.service.spec.tsclient/src/app/services/chart.service.tsclient/src/app/services/config.service.spec.tsclient/src/app/services/config.service.tsclient/src/app/services/user.service.tsclient/src/app/types/chart.types.tsclient/src/main.tsclient/src/test-utils/chart-test-harness.tsdocs/plans/reusable-charts.plan.mdlibs/chartjs-financial/package.jsonlibs/indy-charts/README.mdlibs/indy-charts/api/client.spec.tslibs/indy-charts/api/static.spec.tslibs/indy-charts/charts/chart-manager.spec.tslibs/indy-charts/config/index.tslibs/indy-charts/data/transformers.spec.tslibs/indy-charts/eslint.config.tslibs/indy-charts/helpers/apply-selection-tokens.spec.tslibs/indy-charts/helpers/apply-selection-tokens.tslibs/indy-charts/helpers/calculate-optimal-bars.spec.tslibs/indy-charts/helpers/calculate-optimal-bars.tslibs/indy-charts/helpers/create-default-selection.spec.tslibs/indy-charts/helpers/create-default-selection.tslibs/indy-charts/helpers/index.tslibs/indy-charts/index.tslibs/indy-charts/package.jsonscripts/setup-linux.shtests/playwright/website.spec.tstests/vitepress/.vitepress/theme/components/indy-demo-utils.tstests/vitepress/package.json
💤 Files with no reviewable changes (2)
- client/src/app/services/config.service.ts
- client/src/app/services/config.service.spec.ts
✅ Files skipped from review due to trivial changes (2)
- client/src/app/services/api.service.ts
- docs/plans/reusable-charts.plan.md
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/indy-charts/config/index.ts
- libs/indy-charts/package.json
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
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
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.
Implements the reusable charting library extraction from Issue #452. Extracts financial chart logic from the Angular application into standalone workspace packages and adds a VitePress documentation site with live chart demos.
What changed
New packages
libs/chartjs-financial/(@facioquo/chartjs-chart-financial) — standalone Chart.js extension for candlestick, OHLC, and volume chart typeslibs/indy-charts/(@facioquo/indy-charts) — reusable chart abstractions (OverlayChart,OscillatorChart,ChartManager), API client (createApiClient()), and setup helper (setupIndyCharts())VitePress documentation site (
tests/vitepress/)IndyOverlayDemo.vue/IndyIndicatorsDemo.vuecomponentsdata-testidhooks for Playwrightresponsive: truePlaywright E2E tests (
tests/playwright/)Review feedback fixes
ChartManager.settingsmade private with read-only getter to prevent mutation bypassingupdateTheme()ExtendedChartDatasetdeduplicated intoconfig/types.ts(was declared twice)addExtraBars()now skips weekends so padding aligns with trading sessionsoscillator-chart.tsseed.spec.tsuses relative path instead of hardcoded URLRelated issues
Review notes
The
tests/vitepress/andtests/playwright/workspaces require the backend stack (Azurite, Azure Functions, Web API) running locally to exercise live chart demos. All Playwright tests run against a built-and-previewed static site (pnpm run build && pnpm run preview) so they pass without a live backend.Package structure
Client-side dependency direction is strictly one way:
client→indy-charts→chartjs-financial. Neither library package depends on Angular.