feat(TEA-84): add PR gate for lint, type-check, build, and Docker health#42
Open
Mohamed-Elshesheny wants to merge 3 commits intomainfrom
Open
feat(TEA-84): add PR gate for lint, type-check, build, and Docker health#42Mohamed-Elshesheny wants to merge 3 commits intomainfrom
Mohamed-Elshesheny wants to merge 3 commits intomainfrom
Conversation
…dd Docker healthcheck to Dockerfile
📝 WalkthroughWalkthroughAdds a CI workflow that installs deps, runs lint/type-check/build, and exercises Docker Compose stacks with health waits and teardown; augments release workflow to run lint/check/build before release; Dockerfile nginx stage now created via heredoc and includes an HTTP HEALTHCHECK; several TypeScript typings and microtask scheduling adjustments across UI/shared packages. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions Runner
participant Cache as Turbo/NPM Cache
participant PNPM as pnpm
participant Docker as Docker Engine
participant Compose as docker-compose
participant Service as App Containers
GH->>Cache: restore cache
GH->>PNPM: pnpm install, lint, check-types, build
PNPM-->>GH: results/artifacts
GH->>Docker: build images
Docker-->>GH: images
GH->>Compose: start stack (with health-wait)
Compose->>Service: container start
Service-->>Compose: health checks
Compose-->>GH: healthy / timeout
alt failure
Compose->>GH: capture status & logs
end
GH->>Compose: teardown (including volumes)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
55-57: Split pre-release verification into separate steps for clearer failures.Line 56 is correct, but splitting lint/types/build into individual steps gives better failure visibility and rerun ergonomics.
Proposed refactor
- - name: Verify workspace (lint, types, build) - run: pnpm lint && pnpm check-types && pnpm build + - name: Verify workspace (lint) + run: pnpm lint + + - name: Verify workspace (types) + run: pnpm check-types + + - name: Verify workspace (build) + run: pnpm build🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 55 - 57, The workflow currently runs a combined step named "Verify workspace (lint, types, build)" that chains pnpm lint && pnpm check-types && pnpm build; change this to three separate steps named e.g. "Lint", "Type check", and "Build" that run pnpm lint, pnpm check-types, and pnpm build respectively (replacing the combined step) so failures are isolated and can be re-run individually—update the step names and run commands in the release.yml job accordingly.
🤖 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/ci.yml:
- Line 13: Add an explicit least-privilege GITHUB_TOKEN permissions block to the
workflow (instead of relying on defaults) to restrict token scope for the
read/build/test job(s); update the top-level or job-level definition referenced
by jobs to include a permissions mapping (e.g., set contents: read and packages:
read and any other minimal read permissions your CI needs) so the GITHUB_TOKEN
is not granted broader rights.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 55-57: The workflow currently runs a combined step named "Verify
workspace (lint, types, build)" that chains pnpm lint && pnpm check-types &&
pnpm build; change this to three separate steps named e.g. "Lint", "Type check",
and "Build" that run pnpm lint, pnpm check-types, and pnpm build respectively
(replacing the combined step) so failures are isolated and can be re-run
individually—update the step names and run commands in the release.yml job
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5f77c39-ab0d-4ae9-bb91-2d296d6aa52a
📒 Files selected for processing (3)
.github/workflows/ci.yml.github/workflows/release.ymlDockerfile
| group: ci-${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: |
There was a problem hiding this comment.
Set explicit least-privilege GITHUB_TOKEN permissions.
This workflow performs read/build/test actions only. Add explicit read-only permissions to avoid inheriting broader defaults.
Proposed hardening
name: CI
@@
concurrency:
group: ci-${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true
+
+permissions:
+ contents: read🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml at line 13, Add an explicit least-privilege
GITHUB_TOKEN permissions block to the workflow (instead of relying on defaults)
to restrict token scope for the read/build/test job(s); update the top-level or
job-level definition referenced by jobs to include a permissions mapping (e.g.,
set contents: read and packages: read and any other minimal read permissions
your CI needs) so the GITHUB_TOKEN is not granted broader rights.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shared/src/debounce.ts`:
- Line 7: The debounce generic currently constrains F with (...args: never[])
which forbids any non-zero-arg functions; update the generic to allow arbitrary
argument tuples by introducing a Params tuple and constraining F with (...args:
Params) => unknown (e.g., add a generic Params extends unknown[] and use F
extends (...args: Params) => unknown) and use that Params for any internal
argument typings so debounce(func, waitFor) accepts callbacks with parameters
(such as the 3-arg handler used in SketchDialog.tsx) while preserving type
safety for func and returned wrapper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32cfb867-4c9d-48d2-9a6e-cffd5ebcb8a5
📒 Files selected for processing (3)
Dockerfilepackages/lib/src/data/tree.tspackages/shared/src/debounce.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
|
|
||
| 'use client'; | ||
| export const debounce = <F extends (...args: any[]) => any>(func: F, waitFor: number) => { | ||
| export const debounce = <F extends (...args: never[]) => unknown>(func: F, waitFor: number) => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Current debounce signature:"
rg -n -C2 'export const debounce = <F extends \(\.\.\.args: never\[\]\) => unknown>' packages/shared/src/debounce.ts
echo
echo "Potentially broken debounce callsites (callbacks with at least one declared param):"
rg -nP --type=ts --type=tsx -C2 '\bdebounce\s*\(\s*(?:async\s*)?\((?!\s*\))[^)]*\)\s*=>'Repository: TeamCoderz/WordyMe
Length of output: 402
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verifying debounce signature ==="
cat -n packages/shared/src/debounce.ts | head -20
echo ""
echo "=== Searching for debounce callsites ==="
rg -n 'debounce' --type ts -A 3 packages/editor/src/components/Dialogs/SketchDialog.tsx | head -50
echo ""
echo "=== Finding all debounce usages ==="
rg -n '\bdebounce\s*\(' --type ts --type rust -C 2 | head -100Repository: TeamCoderz/WordyMe
Length of output: 1738
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== All imports of debounce in repo ==="
rg -n "import.*debounce" --type ts
echo ""
echo "=== All debounce function calls ==="
rg -n "debounce\(" --type ts -B 1 -A 5
echo ""
echo "=== Check if there are TypeScript build errors ==="
if [ -f "tsconfig.json" ]; then
echo "Found tsconfig.json"
head -20 tsconfig.json
fiRepository: TeamCoderz/WordyMe
Length of output: 1984
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking SketchDialog full debounce call context ==="
sed -n '467,480p' packages/editor/src/components/Dialogs/SketchDialog.tsx
echo ""
echo "=== Check package.json for build command ==="
if [ -f "package.json" ]; then
jq '.scripts | select(.build != null)' package.json 2>/dev/null || echo "No build script found"
fi
echo ""
echo "=== Checking the return type from debounce ==="
sed -n '7,15p' packages/shared/src/debounce.ts
echo ""
echo "=== Check how saveToLocalStorage is used after debounce ==="
sed -n '467,485p' packages/editor/src/components/Dialogs/SketchDialog.tsxRepository: TeamCoderz/WordyMe
Length of output: 2190
never[] constraint rejects all non-zero-argument callbacks.
The signature <F extends (...args: never[]) => unknown> only accepts functions with zero parameters (or effectively uncallable functions). The callsite in packages/editor/src/components/Dialogs/SketchDialog.tsx (line 468) passes a callback with 3 parameters:
async (elements: readonly ExcalidrawElement[], __appState: AppState, files: BinaryFiles) => { ... }This will fail TypeScript type-checking because the parameter types cannot satisfy the never constraint.
Fix: Use a generic tuple type instead:
Suggested fix
-export const debounce = <F extends (...args: never[]) => unknown>(func: F, waitFor: number) => {
+export const debounce = <Args extends unknown[], R>(
+ func: (...args: Args) => R,
+ waitFor: number,
+) => {
let timeout: ReturnType<typeof setTimeout> | null = null;
- return (...args: Parameters<F>): void => {
+ return (...args: Args): void => {
if (timeout !== null) {
clearTimeout(timeout);
}
timeout = setTimeout(() => func(...args), waitFor);
};
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared/src/debounce.ts` at line 7, The debounce generic currently
constrains F with (...args: never[]) which forbids any non-zero-arg functions;
update the generic to allow arbitrary argument tuples by introducing a Params
tuple and constraining F with (...args: Params) => unknown (e.g., add a generic
Params extends unknown[] and use F extends (...args: Params) => unknown) and use
that Params for any internal argument typings so debounce(func, waitFor) accepts
callbacks with parameters (such as the 3-arg handler used in SketchDialog.tsx)
while preserving type safety for func and returned wrapper.
Contributor
Author
@ibastawisi follow-up needed (
|
…ty issues with ESLint 10
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/ui/src/hooks/use-throttle.ts (1)
17-20: Add a cleanup guard for the queued microtask updateThe queued callback can still run after cleanup. Add a cancellation flag so Line 18 doesn’t write state after unmount/teardown.
♻️ Suggested adjustment
useEffect(() => { + let cancelled = false; const now = Date.now(); if (lastUpdated.current && now >= lastUpdated.current + interval) { lastUpdated.current = now; queueMicrotask(() => { - setThrottledValue(value); + if (!cancelled) setThrottledValue(value); }); - return; + return () => { + cancelled = true; + }; } const id = window.setTimeout(() => { lastUpdated.current = now; setThrottledValue(value); }, interval); - return () => window.clearTimeout(id); + return () => { + cancelled = true; + window.clearTimeout(id); + }; }, [value, interval]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/hooks/use-throttle.ts` around lines 17 - 20, The queued microtask in useThrottle that calls setThrottledValue via queueMicrotask can run after unmount; add a local cancelled flag (e.g., let cancelled = false) inside the effect, wrap the queueMicrotask callback to check !cancelled before calling setThrottledValue, and set cancelled = true in the effect cleanup to prevent state updates after teardown; reference the useThrottle effect, the queueMicrotask call and setThrottledValue when making the change.packages/ui/src/components/color-picker.tsx (1)
188-195: Consider guarding queued state sync with effect cleanupBecause updates are deferred (Lines 191-195), add a cancellation guard so stale queued callbacks don’t write after teardown.
♻️ Suggested adjustment
useEffect(() => { const text = getColorAsHsva(textColor || defaultTextColor); const bg = getColorAsHsva(backgroundColor || defaultBackgroundColor); const border = getColorAsHsva(borderColor || defaultBorderColor); + let cancelled = false; queueMicrotask(() => { - setTextColorHsv(text); - setBgColorHsv(bg); - setBorderColorHsv(border); + if (cancelled) return; + setTextColorHsv(text); + setBgColorHsv(bg); + setBorderColorHsv(border); }); + return () => { + cancelled = true; + }; }, [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/color-picker.tsx` around lines 188 - 195, The queued state updates scheduled with queueMicrotask can run after the component unmounts causing stale writes; inside the effect that computes text/bg/border hsv (where getColorAsHsva and queueMicrotask are used) introduce a cancellation guard (e.g., a local mounted/isActive flag) that is set true at start and flipped false in the effect cleanup, and only call setTextColorHsv, setBgColorHsv, and setBorderColorHsv inside the queued callback if the flag is still true; this prevents the microtask from writing state after teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/components/tree.tsx`:
- Line 92: The nested TreeContext.Provider in TreeItem currently sets value to
only { indent, currentItem } which drops the existing tree property; update the
provider to merge the previous context with the overrides (preserve the existing
tree) so descendants like TreeDragLine can still access tree. Locate the
TreeItem usage of TreeContext.Provider and change the value to spread the
previous context (or use the TreeContextValue type) and then set/override indent
and currentItem while keeping tree intact.
In `@packages/ui/src/theme/theme-provider.tsx`:
- Around line 119-122: The fallback currently hardcodes 'new-york' which ignores
the provider's defaultTheme and doesn't realign color; update the recovery path
so that when THEME_BY_VALUE[theme] is falsy you queueMicrotask to setThemeState
to the configured defaultTheme (not the hardcoded 'new-york') and also update
the corresponding color state (e.g., call setColorState or the function that
syncs color) based on THEME_BY_VALUE[defaultTheme]. Use the provider's
defaultTheme variable/name from this component and THEME_BY_VALUE to derive the
correct color mapping so both theme and color are restored consistently.
---
Nitpick comments:
In `@packages/ui/src/components/color-picker.tsx`:
- Around line 188-195: The queued state updates scheduled with queueMicrotask
can run after the component unmounts causing stale writes; inside the effect
that computes text/bg/border hsv (where getColorAsHsva and queueMicrotask are
used) introduce a cancellation guard (e.g., a local mounted/isActive flag) that
is set true at start and flipped false in the effect cleanup, and only call
setTextColorHsv, setBgColorHsv, and setBorderColorHsv inside the queued callback
if the flag is still true; this prevents the microtask from writing state after
teardown.
In `@packages/ui/src/hooks/use-throttle.ts`:
- Around line 17-20: The queued microtask in useThrottle that calls
setThrottledValue via queueMicrotask can run after unmount; add a local
cancelled flag (e.g., let cancelled = false) inside the effect, wrap the
queueMicrotask callback to check !cancelled before calling setThrottledValue,
and set cancelled = true in the effect cleanup to prevent state updates after
teardown; reference the useThrottle effect, the queueMicrotask call and
setThrottledValue when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e37ce97f-6dbd-4223-ae74-71f6545604db
📒 Files selected for processing (12)
packages/eslint-config/react-internal.jspackages/ui/src/components/color-picker.tsxpackages/ui/src/components/dynamic-icon.tsxpackages/ui/src/components/icon-picker.tsxpackages/ui/src/components/image-file-input.tsxpackages/ui/src/components/tree.tsxpackages/ui/src/hooks/use-container-query.tspackages/ui/src/hooks/use-hash.tspackages/ui/src/hooks/use-local-storage.tspackages/ui/src/hooks/use-optimistic.tspackages/ui/src/hooks/use-throttle.tspackages/ui/src/theme/theme-provider.tsx
✅ Files skipped from review due to trivial changes (2)
- packages/ui/src/components/icon-picker.tsx
- packages/ui/src/components/dynamic-icon.tsx
|
|
||
| return ( | ||
| <TreeContext.Provider value={{ indent, currentItem: item }}> | ||
| <TreeContext.Provider value={{ indent, currentItem: item } as TreeContextValue<unknown>}> |
There was a problem hiding this comment.
Preserve tree when overriding item context in TreeItem.
On Line 92, the nested provider replaces the context value with only indent + currentItem. Descendants inside TreeItem lose tree, so components like TreeDragLine can unexpectedly resolve tree as undefined and render null.
Proposed fix
function TreeItem<T = unknown>({
item,
className,
asChild,
children,
...props
}: Omit<TreeItemProps<T>, 'indent'>) {
- const { indent } = useTreeContext<T>();
+ const { indent, tree } = useTreeContext();
const itemProps = typeof item.getProps === 'function' ? item.getProps() : {};
const mergedProps = { ...props, ...itemProps };
@@
- <TreeContext.Provider value={{ indent, currentItem: item } as TreeContextValue<unknown>}>
+ <TreeContext.Provider
+ value={{ indent, tree, currentItem: item as ItemInstance<unknown> }}
+ >
<Comp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/tree.tsx` at line 92, The nested
TreeContext.Provider in TreeItem currently sets value to only { indent,
currentItem } which drops the existing tree property; update the provider to
merge the previous context with the overrides (preserve the existing tree) so
descendants like TreeDragLine can still access tree. Locate the TreeItem usage
of TreeContext.Provider and change the value to spread the previous context (or
use the TreeContextValue type) and then set/override indent and currentItem
while keeping tree intact.
Comment on lines
+119
to
+122
| if (THEME_BY_VALUE[theme]) return; | ||
| queueMicrotask(() => { | ||
| setThemeState('new-york'); | ||
| }); |
There was a problem hiding this comment.
Fallback currently ignores provider configuration and can propagate invalid theme state
Line 121 hardcodes 'new-york', so a custom defaultTheme is ignored during recovery. Also, recovery should realign color with the recovered theme.
💡 Proposed fix
useEffect(() => {
if (THEME_BY_VALUE[theme]) return;
- queueMicrotask(() => {
- setThemeState('new-york');
- });
- }, [theme]);
+ setThemeState(defaultTheme);
+ setColorState(THEME_BY_VALUE[defaultTheme]['color-variants'][0].value);
+ }, [theme, defaultTheme]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/theme/theme-provider.tsx` around lines 119 - 122, The
fallback currently hardcodes 'new-york' which ignores the provider's
defaultTheme and doesn't realign color; update the recovery path so that when
THEME_BY_VALUE[theme] is falsy you queueMicrotask to setThemeState to the
configured defaultTheme (not the hardcoded 'new-york') and also update the
corresponding color state (e.g., call setColorState or the function that syncs
color) based on THEME_BY_VALUE[defaultTheme]. Use the provider's defaultTheme
variable/name from this component and THEME_BY_VALUE to derive the correct color
mapping so both theme and color are restored consistently.
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a GitHub Actions PR gate that blocks merges unless
lint,check-types,build, and a full Docker stack healthcheck all pass. Also gives thewebcontainer a self-describingHEALTHCHECKsodocker compose up --waitcan actually verify it, and makesrelease.ymlrefuse to cut a tag on a broken tree.Before this PR, the only workflows in
.github/workflows/wererelease.yml(manual dispatch) andlicense-header.yml(SPDX check). Nothing verified that code landing onmainactually lints, type-checks, builds, or produces a working container image.Related Issues
Linear: TEA-84 — Set up GitHub Actions CI for lint, types, build, and Docker.
Type of Change
Chore / infrastructure (no runtime behavior change).
Changes
.github/workflows/ci.yml— runs onpull_requestandpush: main, with a concurrency group that auto-cancels superseded runs. Two parallel jobs onubuntu-latest:build—actions/checkout@v4→pnpm/action-setup@v4(readspackageManagerfrom rootpackage.json) →actions/setup-node@v4with Node 20 +cache: 'pnpm'→ turbo filesystem cache viaactions/cache@v4→pnpm install --frozen-lockfile→pnpm lint→pnpm check-types→pnpm build. Each command is a separate step so the PR checks UI points directly at the failing gate.docker—docker/setup-buildx-action@v3→docker compose build→docker compose up -d --wait --wait-timeout 180→if: failure()diagnostics dumpingdocker compose psand full logs →if: always()docker compose down -vteardown.Dockerfile— added aHEALTHCHECKto thewebstage (Nginx) using BusyBoxwgetagainsthttp://localhost/. Without it,docker compose up --waitwould treat the web container as healthy the moment it starts, and a broken Nginx config (or missingdist/) would pass CI. The check lives in the Dockerfile (notdocker-compose.yml) so every runtime —docker run, compose, Kubernetes readiness probes — inherits it..github/workflows/release.yml— inserted aVerify workspace (lint, types, build)step betweenInstall dependenciesandConfigure Git user. A manual release against a broken tree now fails before anygit tag/git push.How to Test
Local smoke test (mirrors CI exactly):
pnpm install --frozen-lockfilepnpm lint && pnpm check-types && pnpm builddocker compose builddocker compose up -d --wait --wait-timeout 180docker compose ps— bothwordyme-backendandwordyme-webreporthealthy.docker compose down -vProve the healthcheck actually blocks a broken
webimage:Dockerfile(drop a semicolon in alocationblock).docker compose build web && docker compose up -d --wait --wait-timeout 30wordyme-webgoesstarting→unhealthy,docker compose up --waitexits non-zero within 30s,docker compose psshows the failed state. Revert after confirming.Prove the CI gates block bad PRs (optional, on this or a follow-up branch):
const x: string = 1;to any workspace —Type-checkstep fails.COPY --from=builderline inDockerfile—dockerjob fails with a readable log excerpt from theDump compose state on failurestep.Expected result:
Opening this PR should trigger the new
Lint, type-check, buildandDocker stack healthjobs side-by-side in the PR checks UI; both pass. Re-running CI on the same branch shows turbo cache hits (FULL TURBO/cached) in thebuildstep logs.Summary by CodeRabbit