feature: e2e manual tests#91
Merged
Merged
Conversation
…fig updates - **E2E Test**: Added a comprehensive end-to-end test for the full project lifecycle, including project creation, actor sheet updates, and progress tracking. - **Config Updates**: Enhanced test coverage for `RulesConfig` and `ActorTutelageConfig`, focusing on state synchronization, dynamic UI updates, and auto-save functionality. - **Error Handling**: Improved resilience in actor flag sanitization and error-prone interactions during training configuration changes.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a Playwright E2E infrastructure and many end-to-end tests, supporting setup/teardown, fixtures, helpers, data seeding, CI verification, and related scripts; also includes runtime/type refactors and unit-test adjustments to enable deterministic E2E runs and module workflows. (48 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Playwright Runner
participant Browser as Browser Page
participant Foundry as Foundry App
participant FS as File System
Runner->>Browser: launch browser context (env loaded)
Browser->>Foundry: navigate to baseURL (/setup)
Foundry-->>Browser: render setup UI (may request admin)
Browser->>Foundry: perform auth / join flows (adminPassword optional)
Browser->>Foundry: create world, configure system/modules
Foundry-->>Browser: world launched -> /join -> /game (game.ready)
Browser->>Foundry: activate target module via settings UI
Browser->>FS: write authenticated storageState (e2e/.auth/user.json)
Runner->>Browser: persist state for subsequent tests
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 unit tests (beta)
✨ Simplify code
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. Review rate limit: 0/1 reviews remaining, refill in 28 minutes and 57 seconds.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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/apps/components/RulesConfig.svelte (1)
10-24:⚠️ Potential issue | 🟠 MajorAdd parent-to-child sync to prevent stale state after
rulesreplacement.Line 10 initializes
internalRulesonce, and Lines 22-24 only push updates outward. When the parent reassignsrules(e.g., during settings import at line 107 of WorldSettingsConfig.svelte), this component keeps stale local state becauseinternalRulesis never rehydrated.Suggested fix
let internalRules = $state<SystemRules>(rules || { nonBulkMethod: 'roll', bulkMethod: 'mathematical', rollMode: 'gmroll', checkDC: 12, checkFormula: '', critDoubleStrategy: 'never', critThreshold: 20, notificationLevel: 'info', bulkExpectedFormula: '' }); + // Parent -> child sync (e.g., parent resets/reloads settings) + $effect(() => { + if (rules && rules !== internalRules) { + internalRules = rules; + } + }); + $effect(() => { rules = internalRules; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/apps/components/RulesConfig.svelte` around lines 10 - 24, internalRules is only initialized once and only pushed outwards via the existing $effect that sets rules = internalRules, so when the parent replaces rules the child stays stale; add a parent-to-child sync $effect that watches rules and updates internalRules (e.g. $effect(() => { if (rules && rules !== internalRules) internalRules = structuredClone(rules); })) using a deep copy (structuredClone or equivalent) to fully rehydrate SystemRules and avoid reference sharing or infinite loops; reference internalRules, rules, and the existing $effect to locate where to add this new reactive sync.
🧹 Nitpick comments (6)
.gitignore (1)
11-15: Minor: verify Playwright artifact directory names (and root-relative ignores).You’re ignoring:
/playwright-report//test-results//.enve2e/.auth/Please double-check that your Playwright config/output actually writes to
test-results/(vs something likeplaywright-results/), and that these folders are always created at the repo root (because of the leading/). If there’s any chance output paths differ (or you run Playwright from a subdir / monorepo), consider adding the alternative directory names as well or dropping the leading/for broader coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 11 - 15, The .gitignore entries use root-relative names (/playwright-report/, /test-results/, /.env, e2e/.auth/) that may not match actual Playwright outputs or monorepo/run-from-subdir setups; verify the Playwright config (outputDir/paths) and CI jobs to confirm whether artifacts are written to test-results, playwright-report, playwright-results, or other names and whether they are created at the repo root. Update .gitignore to include the actual artifact directory names (e.g., add playwright-results or other alternatives) and consider removing the leading slash from entries you want to ignore anywhere in the repo (or keep it if you intend root-only ignores); also normalize the .env entry to the correct form (.env) if it should be ignored anywhere. Ensure e2e/.auth/ matches the actual location (or add .auth/ without a leading path for broader coverage).tests/apps/tabs/ActorTutelageConfig.test.ts (1)
106-110: NarrowgetFlagmocking by key to avoid type bleed.Line 107 currently returns an array for every key, so
learningModeEnabledis also fed a non-boolean value in this test. That can hide regressions in boolean-specific behavior.Suggested test mock tightening
- const mockActor = { - getFlag: vi - .fn() - .mockReturnValue([{ name: "To Delete", modifier: 1, costs: {}, categories: [] }]), - } as any; + const mockActor = { + getFlag: vi.fn().mockImplementation((_scope, key) => { + if (key === "teacherOfferings") { + return [{ name: "To Delete", modifier: 1, costs: {}, categories: [] }]; + } + if (key === "learningModeEnabled") return true; + return null; + }), + } as any;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/apps/tabs/ActorTutelageConfig.test.ts` around lines 106 - 110, The mock for getFlag on mockActor returns the same array for every key which causes boolean flags like learningModeEnabled to receive a non-boolean; update the getFlag mock (the vi.fn() assigned to mockActor.getFlag) to inspect its key argument and return a boolean for keys like "learningModeEnabled" and the array only for the specific key being tested (e.g., "tutelageSkills"), for example by replacing mockReturnValue with mockImplementation((key) => key === "tutelageSkills" ? [{ name: "To Delete", modifier: 1, costs: {}, categories: [] }] : false) so boolean-specific behavior is not masked.e2e/project-lifecycle.spec.ts (1)
100-103: Assert the actual progress change, not just “not 0 / 100”.This passes if the project starts non-zero or stale state survives cleanup, so it doesn't strictly prove that the “Apply Time” step changed anything in this run. Capture the pre-click progress and assert it changes, or assert the exact expected post-grant value if the seed data is deterministic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/project-lifecycle.spec.ts` around lines 100 - 103, The test currently only asserts projectRow.not.toContainText("0 / 100") which can pass for pre-existing non-zero state; before performing the "Apply Time" action capture the current progress text from projectRow (e.g., readProjectProgress or await projectRow.textContent()) into a variable, perform the Apply Time interaction, then assert that the post-action projectRow text is different from the captured pre-action value or equals the known expected value (prefer deterministic assertion if seed data guarantees the exact progress); keep references to projectRow and the Apply Time step in your changes and preserve the page.bringToFront() call.tests/apps/components/UserPreferencesConfig.test.ts (1)
63-102: This only proves DOM toggling, not bound-state propagation.Lines 64-65 create
autoSpend/autoSpendUnits, but the test never re-reads them after the clicks. That means this spec still passes ifUserPreferencesConfig.sveltestops updating its bindable values and only flips local checkbox state. A tiny wrapper likeRulesConfigWrapper.sveltewould let you assert the parent-facing state thatSettingsConfigactually saves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/apps/components/UserPreferencesConfig.test.ts` around lines 63 - 102, The test currently only verifies DOM checkbox state but not that the component updates its bindable props; wrap UserPreferencesConfig in a tiny test wrapper (e.g., RulesConfigWrapper or TestWrapper) that declares autoSpend and autoSpendUnits in its script and binds them to <UserPreferencesConfig bind:autoSpend bind:autoSpendUnits ...>, mount that wrapper instead of UserPreferencesConfig, perform the same clicks, then read the wrapper's bound variables (autoSpend and autoSpendUnits) after await tick() to assert they changed as expected (true/false and contains/omits 'hour'/'day').e2e/data-setup.spec.ts (1)
36-38: Seed by named fixture existence, not pack emptinessChecking
length === 0can skip creation when packs are non-empty but missing required fixtures, causing non-deterministic downstream tests.Proposed change
- if (existingFeats.length === 0) { + if (!existingFeats.some((i) => i.name === "Test Learning Feat")) { const featData = { name: "Test Learning Feat", type: "feat", @@ - if (existingBooks.length === 0) { + if (!existingBooks.some((i) => i.name === "Test Learning Book")) { const bookData = { name: "Test Learning Book", type: "loot", @@ - if (existingTeachers.length === 0) { + if (!existingTeachers.some((i) => i.name === "Test Teacher")) { const teacherData = { name: "Test Teacher", type: "npc",Also applies to: 89-91, 114-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/data-setup.spec.ts` around lines 36 - 38, The current seeding logic uses featPack.getDocuments() and checks existingFeats.length === 0 which wrongly assumes an empty pack means fixtures are missing; instead, change the logic in the blocks around featPack.getDocuments() (and the similar spots at the other occurrences) to look up by the specific fixture identifier/name you expect (e.g., search existingFeats for the fixture name or id) and only create the missing named fixture(s) when they are absent; update the checks that reference existingFeats, replace the length check with a predicate like "existsByName" for the specific fixture, and create the fixture when that predicate is false so packs that are non-empty but missing required fixtures are correctly seeded.e2e/global-setup.ts (1)
149-156: Ensure dependency dialog is fully resolved before savingAfter clicking Activate, wait for the dependency modal to close; otherwise Save click can be intercepted in slower runs.
Proposed change
if (await dependencyDialog.isVisible()) { await dependencyDialog.getByRole("button", { name: "Activate" }).click(); + await expect(dependencyDialog).toBeHidden({ timeout: 15000 }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/global-setup.ts` around lines 149 - 156, After clicking the dependency dialog's "Activate" button, wait for the dialog locator dependencyDialog to become hidden before proceeding so the subsequent Save click isn't intercepted; i.e., after dependencyDialog.getByRole("button", { name: "Activate" }).click() add a wait on dependencyDialog for state "hidden" (with an appropriate timeout) to ensure the modal is fully resolved before continuing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/data-setup.spec.ts`:
- Around line 3-4: The data seeding currently implemented in
e2e/data-setup.spec.ts (the test.describe / test("setup test data", async ({
page }) => { ... }) block) relies on file ordering and must be moved into the
canonical Playwright setup phase: extract the seeding logic out of that spec and
invoke it from global-setup.ts (after any module activation steps) or include it
in the setup project's dependency chain so it runs before other specs; update or
remove the data-setup.spec.ts test to avoid duplicate runs and ensure
global-setup.ts calls the same seed functions so the seeding functions are
executed deterministically prior to test execution.
In `@e2e/foundry.spec.ts`:
- Around line 14-18: The current test only checks presence in game.modules and
can false-pass; update the page.evaluate call used to set isModuleLoaded (the
evaluation that returns !!game.modules.get("thefehrs-learning-manager")) to
instead verify the module is enabled by accessing the module object and
returning its active property (e.g., check
game.modules.get("thefehrs-learning-manager")?.active === true); replace the
boolean coercion with an explicit active check so the
expect(isModuleLoaded).toBe(true) asserts the module is actually enabled.
In `@e2e/global-setup.ts`:
- Around line 159-164: The click on the reload confirmation is racing because
the dialog may not be present yet; before calling
reloadDialog.getByRole(...).click() wait for the dialog element to appear/be
visible (e.g., await reloadDialog.waitFor({ state: "visible" }) or use the
Playwright expect toBeVisible) and then perform the click on the button; update
the sequence around the reloadDialog locator and the getByRole("button", { name:
/Yes/i }) call so the test consistently waits for the dialog before clicking.
In `@e2e/global-teardown.ts`:
- Around line 51-57: The current flow assumes the admin re-login prompt exists
and that submit immediately unlocks the setup UI; instead, first check for the
presence of the admin password input element (e.g., query
'input[name="adminPassword"]') and only call page.fill/page.click when that
element is found and adminPassword is provided; after performing the submit
action, wait for the setup UI to unlock by waiting for a reliable signal (for
example waitForSelector of a post-setup element, waitForSelector that the admin
password input is detached, or waitForURL change away from "/setup") before
calling deleteWorldIfExists(page, worldId); if the input is not present (already
authenticated), skip the login steps and immediately proceed to
deleteWorldIfExists.
- Line 1: The test helper expect is not imported, causing toHaveURL usage
(symbol toHaveURL called via expect) to throw a ReferenceError; add expect to
the Playwright import so expect is in scope before it’s used — e.g., import
expect from "@playwright/test" or include expect in the existing import that
defines teardown; ensure the symbol expect is available in the same module where
toHaveURL is called (referencing expect and toHaveURL) so the runtime can access
it.
In `@e2e/settings.spec.ts`:
- Around line 71-83: After clicking Save Settings (page.getByRole("button", {
name: "Save Settings" }).click()), don't read settings immediately; wait for
persistence before evaluating savedSettings. Replace the immediate
page.evaluate(...) with a small wait that polls game.settings (e.g., via
page.waitForFunction) until the expected values appear, or wait for a known
post-save UI/network signal, then call the page.evaluate(...) that reads
allowedCompendiums/teacherCompendiums/bookCompendiums/rules to populate
savedSettings.
- Around line 13-34: Replace the in-test self-registration logic with a
fail-fast assertion: inside the page.evaluate block that currently defines
moduleId and missing, remove the manual (game as any).settings.register calls
and instead check each key with (game as
any).settings.settings.has(`${moduleId}.${key}`) and throw an error (or return a
failure) if any are missing so the test fails when the module fails to register
settings; reference the existing page.evaluate, moduleId, missing array, and the
game.settings.settings.has check to locate where to change the behavior.
---
Outside diff comments:
In `@src/apps/components/RulesConfig.svelte`:
- Around line 10-24: internalRules is only initialized once and only pushed
outwards via the existing $effect that sets rules = internalRules, so when the
parent replaces rules the child stays stale; add a parent-to-child sync $effect
that watches rules and updates internalRules (e.g. $effect(() => { if (rules &&
rules !== internalRules) internalRules = structuredClone(rules); })) using a
deep copy (structuredClone or equivalent) to fully rehydrate SystemRules and
avoid reference sharing or infinite loops; reference internalRules, rules, and
the existing $effect to locate where to add this new reactive sync.
---
Nitpick comments:
In @.gitignore:
- Around line 11-15: The .gitignore entries use root-relative names
(/playwright-report/, /test-results/, /.env, e2e/.auth/) that may not match
actual Playwright outputs or monorepo/run-from-subdir setups; verify the
Playwright config (outputDir/paths) and CI jobs to confirm whether artifacts are
written to test-results, playwright-report, playwright-results, or other names
and whether they are created at the repo root. Update .gitignore to include the
actual artifact directory names (e.g., add playwright-results or other
alternatives) and consider removing the leading slash from entries you want to
ignore anywhere in the repo (or keep it if you intend root-only ignores); also
normalize the .env entry to the correct form (.env) if it should be ignored
anywhere. Ensure e2e/.auth/ matches the actual location (or add .auth/ without a
leading path for broader coverage).
In `@e2e/data-setup.spec.ts`:
- Around line 36-38: The current seeding logic uses featPack.getDocuments() and
checks existingFeats.length === 0 which wrongly assumes an empty pack means
fixtures are missing; instead, change the logic in the blocks around
featPack.getDocuments() (and the similar spots at the other occurrences) to look
up by the specific fixture identifier/name you expect (e.g., search
existingFeats for the fixture name or id) and only create the missing named
fixture(s) when they are absent; update the checks that reference existingFeats,
replace the length check with a predicate like "existsByName" for the specific
fixture, and create the fixture when that predicate is false so packs that are
non-empty but missing required fixtures are correctly seeded.
In `@e2e/global-setup.ts`:
- Around line 149-156: After clicking the dependency dialog's "Activate" button,
wait for the dialog locator dependencyDialog to become hidden before proceeding
so the subsequent Save click isn't intercepted; i.e., after
dependencyDialog.getByRole("button", { name: "Activate" }).click() add a wait on
dependencyDialog for state "hidden" (with an appropriate timeout) to ensure the
modal is fully resolved before continuing.
In `@e2e/project-lifecycle.spec.ts`:
- Around line 100-103: The test currently only asserts
projectRow.not.toContainText("0 / 100") which can pass for pre-existing non-zero
state; before performing the "Apply Time" action capture the current progress
text from projectRow (e.g., readProjectProgress or await
projectRow.textContent()) into a variable, perform the Apply Time interaction,
then assert that the post-action projectRow text is different from the captured
pre-action value or equals the known expected value (prefer deterministic
assertion if seed data guarantees the exact progress); keep references to
projectRow and the Apply Time step in your changes and preserve the
page.bringToFront() call.
In `@tests/apps/components/UserPreferencesConfig.test.ts`:
- Around line 63-102: The test currently only verifies DOM checkbox state but
not that the component updates its bindable props; wrap UserPreferencesConfig in
a tiny test wrapper (e.g., RulesConfigWrapper or TestWrapper) that declares
autoSpend and autoSpendUnits in its script and binds them to
<UserPreferencesConfig bind:autoSpend bind:autoSpendUnits ...>, mount that
wrapper instead of UserPreferencesConfig, perform the same clicks, then read the
wrapper's bound variables (autoSpend and autoSpendUnits) after await tick() to
assert they changed as expected (true/false and contains/omits 'hour'/'day').
In `@tests/apps/tabs/ActorTutelageConfig.test.ts`:
- Around line 106-110: The mock for getFlag on mockActor returns the same array
for every key which causes boolean flags like learningModeEnabled to receive a
non-boolean; update the getFlag mock (the vi.fn() assigned to mockActor.getFlag)
to inspect its key argument and return a boolean for keys like
"learningModeEnabled" and the array only for the specific key being tested
(e.g., "tutelageSkills"), for example by replacing mockReturnValue with
mockImplementation((key) => key === "tutelageSkills" ? [{ name: "To Delete",
modifier: 1, costs: {}, categories: [] }] : false) so boolean-specific behavior
is not masked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a3f3c57-f71e-49a5-9376-6f05f9a06edb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
.env.example.gitignore.prettierignoreDEVELOPMENT.mde2e/data-setup.spec.tse2e/foundry.spec.tse2e/global-setup.tse2e/global-teardown.tse2e/helpers.tse2e/overview.spec.tse2e/project-lifecycle.spec.tse2e/settings.spec.tse2e/tsconfig.jsonpackage.jsonplaywright.config.tssrc/apps/components/RulesConfig.sveltesrc/apps/dialogs/GrantTimeDialog.sveltesrc/apps/dialogs/InstructorSelectionDialog.sveltesrc/apps/tabs/ActorTutelageConfig.sveltetests/apps/SettingsConfig.test.tstests/apps/components/RulesConfig.test.tstests/apps/components/RulesConfigWrapper.sveltetests/apps/components/UserPreferencesConfig.test.tstests/apps/tabs/ActorTutelageConfig.test.tstests/apps/tabs/PartyTab.test.tsvitest.config.ts
…y5e V2 - Automatically start the Vite dev server on port 30001 during e2e tests. - Update locators to support ApplicationV2 and Tidy5e's specific DOM structure. - Configure Tidy5e as the default sheet for test actors to enable the Group Learning tab. - Fix project data schema (use 'target' instead of 'totalProgress') and improve data setup. - Enhance test reliability with state synchronization delays and more specific scoping for settings.
- Enhance data setup to include near-complete project with stashed effects and weapon type. - Add PC actors to 'Test Group' members for Party Tab visibility. - Implement completion.spec.ts to verify full restoration of original item state (name, type, effects) and removal of learning activities.
…rency deduction - Implement tutelage.spec.ts to verify instructor/book filters, modifiers, and fee deductions. - Fix bug in deductCurrency where mixed string/number types caused deduction failures. - Enhance ActorProxy.updateCurrency to use direct path updates for improved reliability in Foundry. - Expose ProjectEngine API for programmatic project initialization in E2E tests. - Update data-setup.spec.ts with correct teacher cost IDs (using time units instead of 'gp'). - Add stability wait in helpers.ts before world deletion dialog confirmation.
…ct mapping - Introduce ProxyActorCore interface to eliminate 'any' casts in ActorProxy. - Standardize ProjectMappedData in project-item.ts and unify usage across Party Tab. - Fix bug in deductCurrency making change with EP/PP even when actor has zero balance. - Add 'typecheck' script to package.json for manual TS validation. - Update tests to align with enhanced ActorProxy API and project mapping.
- Replace deprecated global 'CompendiumCollection' with 'foundry.documents.collections.CompendiumCollection'. - Update 'core.sourceId' flag usage to prefer '_stats.compendiumSource' and avoid 'getFlag' warnings. - Implement 'e2e/fixtures.ts' to automatically fail Playwright tests upon detecting deprecation warnings in the console. - Update unit and E2E tests to align with namespaced Foundry APIs and updated flag access patterns.
… warnings - Add e2e/resolution.spec.ts to verify bulk vs separate training resolution. - Fix ActivityManager.injectActivities to reuse existing IDs, avoiding dnd5e v3 migration errors and console warnings. - Expand data setup with PC 3, PC 4 and proper activity initialization for projects.
- Automatically fail tests on critical warnings (deprecations, migration errors). - Log browser errors and non-critical warnings to the host console. - Ignore known harmless noise (hardware acceleration, Vite logs).
- Fix 'description' property to align with ActivityData5e type. - Restore defensive checks for timeUnits and GM status. - Ensure syncAllProjectActivities correctly handles both Collection and Array actor lists. - Fix logic to properly increment failedCount on update errors. - Ensure projectData warning is logged when expected by tests.
- Add e2e/time-bank.spec.ts to verify 'Spend All' and 'Auto-Spend' logic. - Verify confirmation dialogs and automatic progress gain from bank updates. - Verify warning when multiple projects exist during auto-spend.
- Implement 'e2e/gm-controls.spec.ts' to verify GM's ability to override progress, target, and abort projects via the Party Tab. - Update 'project-engine.test.ts' to include new render flag validation. - Enhance debug logging for activity creation in 'core/constants.ts' and fix null value defaults for activity targets and ranges. - Expand test coverage in 'settings.spec.ts' with additional notes on compendium config UI elements.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
e2e/global-teardown.ts (1)
20-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSetup unlock check is still ineffective before deletion.
Line 24 waits for
/setup, which is already true pre-submit; teardown can continue without confirming admin unlock.🛠️ Proposed fix
- if (await passwordInput.isVisible()) { - if (adminPassword) { - await passwordInput.fill(adminPassword); - await page.click('button[name="submit"]'); - await page.waitForURL((url) => url.pathname.endsWith("/setup")); - } - } + if ((await passwordInput.count()) > 0) { + if (!adminPassword) { + throw new Error("FOUNDRY_ADMIN_PASSWORD is required when setup login is prompted"); + } + await passwordInput.fill(adminPassword); + await page.getByRole("button", { name: /submit/i }).click(); + await expect(passwordInput).toHaveCount(0, { timeout: 30000 }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/global-teardown.ts` around lines 20 - 25, The teardown currently checks passwordInput.isVisible() and clicks the submit button but then waits for page.waitForURL(url => url.pathname.endsWith("/setup")), which is already true before submit so the unlock is not being confirmed; update the post-submit wait to assert that the app has moved away from the setup route (for example replace the predicate with url => !url.pathname.endsWith("/setup") or use page.waitForNavigation()/page.waitForSelector to detect an element visible only after unlock) so the code surrounding passwordInput.isVisible(), passwordInput.fill(adminPassword) and page.click('button[name="submit"]') reliably waits for the admin-unlocked state before continuing.e2e/global-setup.ts (1)
252-257:⚠️ Potential issue | 🟠 MajorWait for the reload confirmation before clicking "Yes".
The confirmation click can still race the dialog render and fail intermittently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/global-setup.ts` around lines 252 - 257, The click on the reload confirmation can race the dialog render; before calling reloadDialog.getByRole(...).click(), wait for the dialog to be visible using the Playwright locator wait or assertion (e.g., call reloadDialog.waitFor({ state: "visible" }) or await expect(reloadDialog).toBeVisible()) so the element is present and stable, then perform the getByRole("button", { name: /Yes/i }).click() on the same reloadDialog locator.
🧹 Nitpick comments (11)
src/logic/actor-proxy.ts (1)
38-40: 💤 Low valueNon-null assertion on
idcould throw at runtime.If
this.actor.idisundefined(e.g., for a newly created but unsaved actor), returningundefined!asstringwill propagate silently and may cause downstream issues.Consider a fallback similar to the
namegetter pattern, or document that the proxy requires a persisted actor.Suggested defensive fallback
get id(): string { - return this.actor.id!; + return this.actor.id ?? ""; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/logic/actor-proxy.ts` around lines 38 - 40, The get id() getter in ActorProxy currently uses a non-null assertion (this.actor.id!) which can throw at runtime; update the get id() method in the ActorProxy class to avoid the non-null assertion by either returning a defensive fallback like this.actor.id ?? '<unsaved-actor-id>' (matching the existing name getter pattern) or explicitly throw a clear Error (e.g., "ActorProxy requires persisted actor: missing id") so callers fail fast; modify the get id() implementation to use this.actor.id ?? fallbackOrThrow and ensure behavior is consistent with the name getter or is documented as requiring a persisted actor.e2e/helpers.ts (2)
23-23: 💤 Low valueConsider renaming the style element id.
The id
"gemini-disable-tour-style"appears to be an artifact from AI code generation. Consider renaming it to something more contextually appropriate like"e2e-disable-tour-style".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers.ts` at line 23, Rename the DOM style element id assignment to a context-appropriate string: change the assignment to style.id = "e2e-disable-tour-style" (currently "gemini-disable-tour-style") so the created element reflects E2E tests; update any other references or selectors that rely on the old id to use "e2e-disable-tour-style" (look for usages of style.id or document.getElementById("gemini-disable-tour-style") and update them accordingly).
59-59: ⚡ Quick winReplace hardcoded wait with explicit condition.
waitForTimeout(1000)is a fixed delay that can cause flakiness. Consider waiting for the confirmation dialog to be fully visible instead.♻️ Suggested improvement
// Click "Delete World" in the context menu const deleteOption = page.locator(".context-item").filter({ hasText: "Delete World" }); await deleteOption.click(); - await page.waitForTimeout(1000); - // Handle the confirmation dialog with the random code const dialog = page .locator("dialog,div,section,form") .filter({ has: page.getByRole("heading", { name: `Delete World: ${worldId}` }) }) .last(); - await expect(dialog).toBeVisible(); + await expect(dialog).toBeVisible({ timeout: 5000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers.ts` at line 59, Replace the hardcoded delay await page.waitForTimeout(1000) with an explicit wait for the confirmation dialog to appear; locate the usage of page.waitForTimeout in e2e/helpers.ts and swap it for a conditional wait such as await page.waitForSelector('<confirmation-dialog-selector>', { state: 'visible' }) or await page.locator('<confirmation-dialog-selector>').waitFor({ state: 'visible' }), using the actual confirmation dialog selector or test id used elsewhere in the tests (and optionally wait for any confirm button or expected text to be enabled) so the test proceeds only when the dialog is truly visible.e2e/project-lifecycle.spec.ts (2)
156-177: ⚡ Quick winMonkey-patch duplicates logic from time-bank.spec.ts.
This is the same pattern as in
e2e/time-bank.spec.ts. Consider extracting a shared helper function to reduce duplication and ensure consistent behavior across tests.♻️ Extract shared helper to e2e/helpers.ts
// In e2e/helpers.ts export async function patchAutoTrainForGM(page: Page, moduleId: string, projectNameFilter?: string) { await page.evaluate( ({ moduleId, projectNameFilter }) => { const ProjectEngine = (game as any).modules.get(moduleId).api.ProjectEngine; ProjectEngine.handleAutoTrainSignal = async function () { const actor = (game as any).user.character; if (!actor) return; let projects = actor.items.filter((i: any) => i.getFlag(moduleId, "isLearningProject")); if (projectNameFilter) { projects = projects.filter((p: any) => p.name.includes(projectNameFilter)); } if (projects.length >= 1) { await ProjectEngine.processSpendAll(projects[0], ["hour", "day", "workweek", "week"]); } }; }, { moduleId, projectNameFilter }, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/project-lifecycle.spec.ts` around lines 156 - 177, The monkey-patched ProjectEngine.handleAutoTrainSignal in project-lifecycle.spec.ts duplicates logic from time-bank.spec.ts; extract this into a shared helper (e.g., patchAutoTrainForGM) that can be imported by both tests, and update the tests to call that helper instead of inlining the patch. The helper should accept the moduleId and an optional project name filter, run page.evaluate to override ProjectEngine.handleAutoTrainSignal, find actor.items with getFlag(moduleId, "isLearningProject"), optionally filter by projectName, and call ProjectEngine.processSpendAll(project, ["hour","day","workweek","week"]) for the first match; replace the inline override in project-lifecycle.spec.ts with a call to this helper.
5-5: 💤 Low valueConsider removing or gating debug console.log statements.
Multiple
console.logstatements are scattered throughout the test for debugging. Consider removing them or gating behind a DEBUG flag for cleaner test output in CI.Also applies to: 188-188, 329-329, 335-335, 349-349, 353-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/project-lifecycle.spec.ts` at line 5, Remove or gate the ad-hoc browser console debug prints (e.g., the page.on("console", (msg) => console.log("BROWSER CONSOLE:", msg.text())) handler and the other scattered console.log calls) so CI output stays clean: either delete these statements or wrap them behind a DEBUG flag check (e.g., if (process.env.DEBUG) { ... }) or use a small helper like debugLog(msg) that no-ops unless process.env.DEBUG is set; update all occurrences of the inline console.log handlers and calls to use that guard/helper so they only run when debugging is enabled.e2e/time-bank.spec.ts (3)
68-82: ⚡ Quick winConsider replacing hardcoded wait with polling assertion.
waitForTimeout(3000)is a fixed delay that can cause flakiness - either failing prematurely on slower systems or unnecessarily slowing tests on faster ones. Consider polling the state instead.♻️ Suggested improvement
- // 6. Verify bank is empty and project progressed - // We'll wait a bit for the async operations to complete - await page.waitForTimeout(3000); - - const stats = await page.evaluate((moduleId) => { + // 6. Verify bank is empty and project progressed + await expect + .poll( + async () => + page.evaluate((moduleId) => { + const actor = (game as any).actors.getName("PC 3"); + return actor.getFlag(moduleId, "bank")?.total; + }, moduleId), + { timeout: 15000 }, + ) + .toBe(0); + + const stats = await page.evaluate((moduleId) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/time-bank.spec.ts` around lines 68 - 82, The test currently uses a fixed sleep (page.waitForTimeout) which causes flakiness; replace it with a polling assertion using page.waitForFunction (or an explicit retry loop) that repeatedly calls the same page.evaluate (using moduleId, actor "PC 3", and the project item lookup) and waits until stats.bankTotal === 0 and stats.progress > 0, with a sensible overall timeout and interval; if the condition isn't met within the timeout fail the test. Ensure you remove the waitForTimeout call and use the evaluated condition (bankTotal and progress) as the predicate for the wait.
120-154: 💤 Low valueMonkey-patch duplicates source logic and may drift.
The patched
handleAutoTrainSignalreimplements the production logic without calling the original. If the real implementation changes (e.g., new validation, different behavior), this test won't catch regressions. Consider either:
- Patching only the
isGMcheck instead of the entire method- Using a test-specific configuration flag that the real implementation respects
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/time-bank.spec.ts` around lines 120 - 154, The test replaces ProjectEngine.handleAutoTrainSignal wholesale which duplicates logic; instead wrap the original: save originalHandle, replace handleAutoTrainSignal with a small wrapper that temporarily bypasses the isGM check (or sets a test-only config flag the real implementation reads), then call await originalHandle.apply(this, arguments) and finally restore any modified globals; reference ProjectEngine.handleAutoTrainSignal, originalHandle and ProjectEngine.processSpendAll to locate code and ensure you only change the isGM gating rather than reimplementing business logic.
175-177: ⚡ Quick winReplace hardcoded wait with polling.
Similar to the earlier comment,
waitForTimeout(5000)should be replaced with a polling assertion for better reliability.♻️ Suggested improvement
- // 3. Verify project progress increased automatically - await page.waitForTimeout(5000); // Give it more time to process + // 3. Verify project progress increased automatically + await expect + .poll( + async () => + page.evaluate((moduleId) => { + const actor = (game as any).actors.getName("PC 3"); + const project = actor.items.find((i: any) => i.name.includes("Time Bank Project")); + return project.getFlag(moduleId, "projectData")?.progress; + }, moduleId), + { timeout: 15000 }, + ) + .toBeGreaterThan(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/time-bank.spec.ts` around lines 175 - 177, Replace the hardcoded sleep at the "Verify project progress increased automatically" step by polling the app state instead of using await page.waitForTimeout(5000); specifically, remove await page.waitForTimeout(5000) and use a polling helper such as Playwright's expect.poll or page.waitForFunction to repeatedly query the project progress (e.g., read the progress value from the DOM or call the same API used elsewhere in the test) until it increases or a timeout is reached; ensure you reference the same selector or accessor used in surrounding assertions so the test fails fast if progress never updates.src/core/activity-manager.ts (1)
119-121: 💤 Low valueDocument the orphan cleanup strategy.
The comment explains why removal is skipped due to dnd5e 3.x migration issues, but orphaned learning activities could accumulate over time if time units are removed from settings. Consider adding a TODO or tracking issue for when the dnd5e migration issue is resolved.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/activity-manager.ts` around lines 119 - 121, The current comment in src/core/activity-manager.ts about skipping removal of orphaned learning activities during dnd5e 3.x migration lacks a recorded plan; update the comment (near the existing note in activity-manager.ts, within the ActivityManager or cleanup-related block) to add a clear TODO and/or link to a tracking issue ID describing the intended orphan cleanup strategy once the dnd5e migration bug is resolved (e.g., how and when to remove entries from system.activities during silent updates, fallback/retention policy, and a reminder to re-run cleanup). Include guidance to revisit functions/methods that manage activities (references to the orphan removal logic in ActivityManager) and ensure the comment names the migration constraint and the action items so future maintainers know to implement the cleanup when the migration issue is fixed.e2e/tutelage.spec.ts (1)
272-272: ⚡ Quick winReplace hardcoded wait with polling.
waitForTimeout(5000)should be replaced with a polling assertion to avoid flakiness.♻️ Suggested improvement
- // 8. Verify GP deducted and time deducted - await page.waitForTimeout(5000); - - const finalStudentData = await page.evaluate((moduleId) => { + // 8. Verify GP deducted and time deducted + await expect + .poll( + async () => + page.evaluate((moduleId) => { + const actor = (game as any).actors.getName("Poor Student"); + return actor.getFlag(moduleId, "bank").total; + }, moduleId), + { timeout: 15000 }, + ) + .toBe(9); + + const finalStudentData = await page.evaluate((moduleId) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tutelage.spec.ts` at line 272, Replace the hardcoded sleep call await page.waitForTimeout(5000); with a polling assertion that waits for the specific condition or element instead of an arbitrary delay; e.g., use page.waitForSelector('your-expected-selector', { timeout: 5000 }) or Playwright's expect.poll to repeatedly check the desired state (visibility/text/state) until success; update the test in tutelage.spec.ts to remove the page.waitForTimeout invocation and use the appropriate selector or predicate so the test reliably waits for the real condition.e2e/global-setup.ts (1)
149-158: Drive dependency checks from the manifest instead of hard-coded module titles.The test asserts both
"Tidy 5e Sheets"and"Spotlight Omnisearch", butpublic/module.jsononly declarestidy5e-sheetas required. Spotlight Omnisearch is treated as optional in the codebase (with fallback to Quick Insert when unavailable), yet the test enforces its presence unconditionally. Reading required module ids from the manifest will avoid drift and false failures when upstream module titles or dependency trees change, and prevent tests from requiring optional dependencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/00-data-setup.spec.ts`:
- Around line 316-317: The completion fixture uses stashedType: "weapon" while
the seeded project is a feat; update the stashedType value where stashedName and
stashedType are set (referenced variables completionProjectName and stashedType)
to "feat" so the completion flow materializes a feat document and exercises the
feat-completion path correctly.
- Around line 35-37: The current seeding checks pack emptiness (using
featPack.getDocuments() and existingFeats.length === 0) which misses cases where
the pack is partially populated; instead, for the feat pack (featPack) locate by
document name (e.g., "Test Learning Feat") and only create that document when a
document with that name is not found; apply the same per-name existence
check-and-create approach to the book and teacher fixtures (the corresponding
pack variables and document names used later) so each required fixture is
inserted if and only if its name is missing.
In `@e2e/completion.spec.ts`:
- Around line 135-138: The current check uses Object.values(activities) and can
miss activities when `activities` is a collection object that stores items under
a `.contents` property; update the logic around the `activities` variable to
first prefer `activities.contents` when present (e.g., let activityValues =
activities?.contents ? Object.values(activities.contents) :
Object.values(activities)), then run the existing .some(...) check against
`activityValues` for the flag `thefehrs-learning-manager` and
`isLearningActivity` to avoid false negatives.
In `@e2e/global-setup.ts`:
- Around line 53-56: The current guard using currentUrl and page.title() lets
the test skip provisioning when the browser lands in an already-launched world
(currentUrl.includes(`/${worldId}/`) or (await page.title()).includes(worldId)),
causing deleteWorldIfExists() and world creation to be skipped; change the flow
so you always navigate/route to the /setup path before any provisioning logic
runs (i.e., remove or bypass the short-circuit that checks for existing world in
the block guarded by currentUrl.includes("/setup"), ensure the code
unconditionally navigates to "/setup" when not already on it, then call
deleteWorldIfExists() and the world creation routines).
- Around line 19-25: The current debug logs in e2e/global-setup.ts leak secret
material by printing rawAdminPassword length and characters; modify the logic
around rawAdminPassword/FOUNDRY_ADMIN_PASSWORD so you never log any part of the
secret — remove the length and first/last-char console.log calls and replace
them with a single boolean-safe message such as "admin password provided:
true/false" (e.g., using !!rawAdminPassword) and ensure no code references or
prints the actual environment value anywhere in the file.
In `@e2e/resolution.spec.ts`:
- Around line 61-71: The test races because page.waitForFunction only waits for
any new message but the later assertion in the newMsgCountBulk calculation
expects exactly 2; update the wait to block until the messages delta equals the
expected count (e.g., replace the predicate in page.waitForFunction to (initial)
=> (game as any).messages.size - initial === 2) and then compute the delta with
page.evaluate (using the same initialMsgCount) to assert exact equality; apply
the same fix to the other occurrence around the separate path (the other
page.waitForFunction/page.evaluate pair) so both wait-for and evaluate use the
exact expected delta rather than “> initial.”
In `@e2e/tutelage.spec.ts`:
- Around line 260-262: The dialog reference obtained earlier (variable dialog)
becomes stale after the sheet is closed/reopened; re-query the dialog locator
after the sheet is reopened and before interacting with it again (i.e., before
calling dialog.locator(...).click() and dialog.getByRole(...).click()). Locate
the fresh dialog instance using the same selector used originally and replace
the stale dialog usage with the newly queried dialog variable so the subsequent
interactions target the reopened dialog.
In `@src/core/constants.ts`:
- Around line 7-9: Replace the ad-hoc // `@ts-ignore` by adding a proper global
type for Logger and then use it; declare an interface for the Logger shape and
augment GlobalThis (e.g. add `declare global { interface GlobalThis { Logger?: {
debug(message: string): void } } }` or a named `Logger` interface) so
`globalThis.Logger` is typed, remove the `@ts-ignore`, and keep the runtime check
`if (typeof globalThis.Logger !== "undefined") globalThis.Logger.debug("Creating
base activity template");`.
---
Duplicate comments:
In `@e2e/global-setup.ts`:
- Around line 252-257: The click on the reload confirmation can race the dialog
render; before calling reloadDialog.getByRole(...).click(), wait for the dialog
to be visible using the Playwright locator wait or assertion (e.g., call
reloadDialog.waitFor({ state: "visible" }) or await
expect(reloadDialog).toBeVisible()) so the element is present and stable, then
perform the getByRole("button", { name: /Yes/i }).click() on the same
reloadDialog locator.
In `@e2e/global-teardown.ts`:
- Around line 20-25: The teardown currently checks passwordInput.isVisible() and
clicks the submit button but then waits for page.waitForURL(url =>
url.pathname.endsWith("/setup")), which is already true before submit so the
unlock is not being confirmed; update the post-submit wait to assert that the
app has moved away from the setup route (for example replace the predicate with
url => !url.pathname.endsWith("/setup") or use
page.waitForNavigation()/page.waitForSelector to detect an element visible only
after unlock) so the code surrounding passwordInput.isVisible(),
passwordInput.fill(adminPassword) and page.click('button[name="submit"]')
reliably waits for the admin-unlocked state before continuing.
---
Nitpick comments:
In `@e2e/helpers.ts`:
- Line 23: Rename the DOM style element id assignment to a context-appropriate
string: change the assignment to style.id = "e2e-disable-tour-style" (currently
"gemini-disable-tour-style") so the created element reflects E2E tests; update
any other references or selectors that rely on the old id to use
"e2e-disable-tour-style" (look for usages of style.id or
document.getElementById("gemini-disable-tour-style") and update them
accordingly).
- Line 59: Replace the hardcoded delay await page.waitForTimeout(1000) with an
explicit wait for the confirmation dialog to appear; locate the usage of
page.waitForTimeout in e2e/helpers.ts and swap it for a conditional wait such as
await page.waitForSelector('<confirmation-dialog-selector>', { state: 'visible'
}) or await page.locator('<confirmation-dialog-selector>').waitFor({ state:
'visible' }), using the actual confirmation dialog selector or test id used
elsewhere in the tests (and optionally wait for any confirm button or expected
text to be enabled) so the test proceeds only when the dialog is truly visible.
In `@e2e/project-lifecycle.spec.ts`:
- Around line 156-177: The monkey-patched ProjectEngine.handleAutoTrainSignal in
project-lifecycle.spec.ts duplicates logic from time-bank.spec.ts; extract this
into a shared helper (e.g., patchAutoTrainForGM) that can be imported by both
tests, and update the tests to call that helper instead of inlining the patch.
The helper should accept the moduleId and an optional project name filter, run
page.evaluate to override ProjectEngine.handleAutoTrainSignal, find actor.items
with getFlag(moduleId, "isLearningProject"), optionally filter by projectName,
and call ProjectEngine.processSpendAll(project,
["hour","day","workweek","week"]) for the first match; replace the inline
override in project-lifecycle.spec.ts with a call to this helper.
- Line 5: Remove or gate the ad-hoc browser console debug prints (e.g., the
page.on("console", (msg) => console.log("BROWSER CONSOLE:", msg.text())) handler
and the other scattered console.log calls) so CI output stays clean: either
delete these statements or wrap them behind a DEBUG flag check (e.g., if
(process.env.DEBUG) { ... }) or use a small helper like debugLog(msg) that
no-ops unless process.env.DEBUG is set; update all occurrences of the inline
console.log handlers and calls to use that guard/helper so they only run when
debugging is enabled.
In `@e2e/time-bank.spec.ts`:
- Around line 68-82: The test currently uses a fixed sleep (page.waitForTimeout)
which causes flakiness; replace it with a polling assertion using
page.waitForFunction (or an explicit retry loop) that repeatedly calls the same
page.evaluate (using moduleId, actor "PC 3", and the project item lookup) and
waits until stats.bankTotal === 0 and stats.progress > 0, with a sensible
overall timeout and interval; if the condition isn't met within the timeout fail
the test. Ensure you remove the waitForTimeout call and use the evaluated
condition (bankTotal and progress) as the predicate for the wait.
- Around line 120-154: The test replaces ProjectEngine.handleAutoTrainSignal
wholesale which duplicates logic; instead wrap the original: save
originalHandle, replace handleAutoTrainSignal with a small wrapper that
temporarily bypasses the isGM check (or sets a test-only config flag the real
implementation reads), then call await originalHandle.apply(this, arguments) and
finally restore any modified globals; reference
ProjectEngine.handleAutoTrainSignal, originalHandle and
ProjectEngine.processSpendAll to locate code and ensure you only change the isGM
gating rather than reimplementing business logic.
- Around line 175-177: Replace the hardcoded sleep at the "Verify project
progress increased automatically" step by polling the app state instead of using
await page.waitForTimeout(5000); specifically, remove await
page.waitForTimeout(5000) and use a polling helper such as Playwright's
expect.poll or page.waitForFunction to repeatedly query the project progress
(e.g., read the progress value from the DOM or call the same API used elsewhere
in the test) until it increases or a timeout is reached; ensure you reference
the same selector or accessor used in surrounding assertions so the test fails
fast if progress never updates.
In `@e2e/tutelage.spec.ts`:
- Line 272: Replace the hardcoded sleep call await page.waitForTimeout(5000);
with a polling assertion that waits for the specific condition or element
instead of an arbitrary delay; e.g., use
page.waitForSelector('your-expected-selector', { timeout: 5000 }) or
Playwright's expect.poll to repeatedly check the desired state
(visibility/text/state) until success; update the test in tutelage.spec.ts to
remove the page.waitForTimeout invocation and use the appropriate selector or
predicate so the test reliably waits for the real condition.
In `@src/core/activity-manager.ts`:
- Around line 119-121: The current comment in src/core/activity-manager.ts about
skipping removal of orphaned learning activities during dnd5e 3.x migration
lacks a recorded plan; update the comment (near the existing note in
activity-manager.ts, within the ActivityManager or cleanup-related block) to add
a clear TODO and/or link to a tracking issue ID describing the intended orphan
cleanup strategy once the dnd5e migration bug is resolved (e.g., how and when to
remove entries from system.activities during silent updates, fallback/retention
policy, and a reminder to re-run cleanup). Include guidance to revisit
functions/methods that manage activities (references to the orphan removal logic
in ActivityManager) and ensure the comment names the migration constraint and
the action items so future maintainers know to implement the cleanup when the
migration issue is fixed.
In `@src/logic/actor-proxy.ts`:
- Around line 38-40: The get id() getter in ActorProxy currently uses a non-null
assertion (this.actor.id!) which can throw at runtime; update the get id()
method in the ActorProxy class to avoid the non-null assertion by either
returning a defensive fallback like this.actor.id ?? '<unsaved-actor-id>'
(matching the existing name getter pattern) or explicitly throw a clear Error
(e.g., "ActorProxy requires persisted actor: missing id") so callers fail fast;
modify the get id() implementation to use this.actor.id ?? fallbackOrThrow and
ensure behavior is consistent with the name getter or is documented as requiring
a persisted actor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d724920e-3166-42f3-9184-0406898ec54b
📒 Files selected for processing (34)
e2e/00-data-setup.spec.tse2e/completion.spec.tse2e/fixtures.tse2e/foundry.spec.tse2e/global-setup.tse2e/global-teardown.tse2e/gm-controls.spec.tse2e/helpers.tse2e/overview.spec.tse2e/project-lifecycle.spec.tse2e/resolution.spec.tse2e/settings.spec.tse2e/time-bank.spec.tse2e/tutelage.spec.tspackage.jsonplaywright.config.tssrc/apps/party-tab.tssrc/apps/tabs/PartyTab.sveltesrc/core/activity-manager.tssrc/core/constants.tssrc/logic/actor-proxy.tssrc/logic/party-tab-logic.tssrc/logic/project-item.tssrc/logic/tab-logic.tssrc/logic/tutelage-resolver.tssrc/main.tssrc/migrations/v2-native-items.tssrc/migrations/v3-tutelage-selection.tstests/core/activity-manager.test.tstests/logic/actor-proxy.test.tstests/logic/project-engine.test.tstests/logic/tutelage-resolver.test.tstests/migrations/v3-tutelage-selection.test.tstests/setup.ts
✅ Files skipped from review due to trivial changes (6)
- tests/logic/project-engine.test.ts
- src/apps/tabs/PartyTab.svelte
- tests/setup.ts
- src/logic/party-tab-logic.ts
- src/logic/project-item.ts
- tests/logic/tutelage-resolver.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/foundry.spec.ts
- e2e/overview.spec.ts
…ate tests accordingly
…nction checks - Updated multiple E2E tests to dynamically wait for specific conditions instead of using hardcoded timeouts, improving test reliability. - Enhanced checks in tutelage.spec.ts, time-bank.spec.ts, and gm-controls.spec.ts to verify state changes and backend updates. - Improved compendium indexing and settings validation logic in settings.spec.ts for stability.
…ation checks - Replace "weapon" references with "feat" in completion.spec.ts for item restoration and type verification. - Refactor data setup to ensure learning books and teachers are created only if missing, improving efficiency. - Enhance logging for dynamically created learning books and teachers for better test debugging.
- Add `returnToSetup` helper to centralize navigation and authentication steps in global-setup and teardown. - Replace in-place updates with recreation for fallback project restoration, ensuring clean data migration for activities. - Simplify completion tests by removing deprecated in-place validation logic. - Update relevant tests and logic to reflect consistent recreation flow and enhance failure logging.
…dling in global-setup
- Fix incorrect dialog targeting by ensuring the last matching dialog is selected in tutelage and resolution specs. - Improve message count verification with stricter condition checks for enhanced test reliability.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/core/activity-manager.test.ts (1)
5-30:⚠️ Potential issue | 🔴 CriticalRewrite mock specifiers to match source imports and use hoisting-safe pattern.
The mock specifiers do not match the actual imports in
ActivityManager. The source file imports./settings.jsand./foundry.js(relative paths fromsrc/core/), but the test mocks../../src/core/settingsand../../src/core/foundry(absolute-style paths). Vitest will not intercept mismatched specifiers, so these mocks will never apply.Additionally,
vi.mock()is hoisted, but the factory functions referencemockUIandmockGame, which are declared after the mocks. This causes a TDZReferenceErrorduring suite setup.Use
vi.hoisted()to safely declare the mocks and factories together, then mock with the correct relative specifiers:const { mockUI, mockGame } = vi.hoisted(() => { const mockUI = { notifications: { info: vi.fn(), warn: vi.fn() } }; const mockGame = { user: { isGM: true }, actors: { contents: [] } }; return { mockUI, mockGame }; }); vi.mock("./settings.js", () => ({ /* ... */ })); vi.mock("./foundry.js", () => ({ getGame: vi.fn(() => mockGame), getUI: vi.fn(() => mockUI) }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/activity-manager.test.ts` around lines 5 - 30, The test mocks use wrong module specifiers and reference variables before they exist; change the vi.mock specifiers to match ActivityManager's imports ("./settings.js" and "./foundry.js") and hoist the mock data using vi.hoisted so mockUI and mockGame are created before the mock factories run; then update the mock factories to return the Settings object (including ID and get implementation for "timeUnits") and foundry functions (getGame -> mockGame, getUI -> mockUI) so the mocks are applied and TDZ errors are avoided.
♻️ Duplicate comments (1)
e2e/00-data-setup.spec.ts (1)
304-306:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMatch the exact seeded project name here.
startsWith()can treat a stale variant as a hit and skip recreating the intendedTest Learning Feat (95/100)fixture, which makes downstream E2E state nondeterministic.🛠️ Proposed fix
- const existingCompletion = pc1.items.find((i) => i.name.startsWith(completionProjectName)); + const existingCompletion = pc1.items.find( + (i) => i.name === `${completionProjectName} (95/100)`, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/00-data-setup.spec.ts` around lines 304 - 306, The test uses a loose startsWith check which can match stale variants; change the code that defines completionProjectName and the lookup (pc1.items.find) to match the exact seeded project name string (e.g., "Test Learning Feat (95/100)") and use a strict equality check (i.e., compare i.name === completionProjectName) instead of startsWith to ensure the exact fixture is detected and recreated when missing.
🧹 Nitpick comments (1)
e2e/helpers.ts (1)
59-59: ⚡ Quick winRemove the fixed 1s sleep here.
The confirmation-dialog assertion already waits for the next state, so this delay only slows teardown and can still flake on slower runs.
♻️ Proposed fix
- await page.waitForTimeout(1000); - // Handle the confirmation dialog with the random code🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers.ts` at line 59, Remove the fixed 1s sleep (the call to page.waitForTimeout(1000)) and rely on the confirmation-dialog assertion to await the next state; delete the page.waitForTimeout(1000) invocation in e2e/helpers.ts (or replace it with a deterministic wait such as waiting for the confirmation dialog locator via locator.waitFor or page.waitForSelector if an explicit wait is needed) so teardown is not slowed and flakiness on slow runs is avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/time-bank.spec.ts`:
- Around line 99-167: The test replaces ProjectEngine.handleAutoTrainSignal
which bypasses real logic and mutates shared world state; instead, call the real
entrypoint: do not overwrite ProjectEngine.handleAutoTrainSignal, ensure the
test runs under a GM context or temporarily stub the isGM guard (e.g., set
game.user.isGM = true during the test) so ProjectEngine.handleAutoTrainSignal
and ProjectEngine.processSpendAll execute normally, and avoid leaking state by
saving and restoring modified settings and actor assignment (capture original
values from game.settings.get(moduleId, ...) and game.user.character and actor
flags like actor.getFlag(moduleId, "bank")/project.getFlag(moduleId,
"projectData") before changes, then restore them in a finally/after block); also
spy/assert on ui.notifications.warn instead of mutating the engine to verify
warning paths.
---
Outside diff comments:
In `@tests/core/activity-manager.test.ts`:
- Around line 5-30: The test mocks use wrong module specifiers and reference
variables before they exist; change the vi.mock specifiers to match
ActivityManager's imports ("./settings.js" and "./foundry.js") and hoist the
mock data using vi.hoisted so mockUI and mockGame are created before the mock
factories run; then update the mock factories to return the Settings object
(including ID and get implementation for "timeUnits") and foundry functions
(getGame -> mockGame, getUI -> mockUI) so the mocks are applied and TDZ errors
are avoided.
---
Duplicate comments:
In `@e2e/00-data-setup.spec.ts`:
- Around line 304-306: The test uses a loose startsWith check which can match
stale variants; change the code that defines completionProjectName and the
lookup (pc1.items.find) to match the exact seeded project name string (e.g.,
"Test Learning Feat (95/100)") and use a strict equality check (i.e., compare
i.name === completionProjectName) instead of startsWith to ensure the exact
fixture is detected and recreated when missing.
---
Nitpick comments:
In `@e2e/helpers.ts`:
- Line 59: Remove the fixed 1s sleep (the call to page.waitForTimeout(1000)) and
rely on the confirmation-dialog assertion to await the next state; delete the
page.waitForTimeout(1000) invocation in e2e/helpers.ts (or replace it with a
deterministic wait such as waiting for the confirmation dialog locator via
locator.waitFor or page.waitForSelector if an explicit wait is needed) so
teardown is not slowed and flakiness on slow runs is avoided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da9b49f8-4a90-4745-91e1-00f41e8ddad5
📒 Files selected for processing (15)
e2e/00-data-setup.spec.tse2e/completion.spec.tse2e/foundry.spec.tse2e/global-setup.tse2e/global-teardown.tse2e/gm-controls.spec.tse2e/helpers.tse2e/resolution.spec.tse2e/settings.spec.tse2e/time-bank.spec.tse2e/tutelage.spec.tssrc/core/constants.tssrc/logic/project-lifecycle.tstests/core/activity-manager.test.tstests/logic/project-engine.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- e2e/foundry.spec.ts
- e2e/global-teardown.ts
- e2e/completion.spec.ts
- tests/logic/project-engine.test.ts
- e2e/global-setup.ts
- e2e/tutelage.spec.ts
- Implement `e2e/multi-user.spec.ts` to verify cross-user signaling, auto-spend, and time distribution functionality. - Add `e2e/player-setup.ts` for reusable player authentication and setup. - Update `time-bank.spec.ts` to capture and restore original state for "Spend All" tests. - Remove redundant static waits in `helpers.ts` for better reliability. - Standardize formatting in `00-data-setup.spec.ts` and enhance project creation checks.
…fig logic - Deleted `player-setup.ts` as it is no longer required. - Moved viewport configuration to a unified location in `playwright.config.ts`. - Updated `multi-user.spec.ts` to directly define player context with a consistent viewport. - Improved error handling in `helpers.ts` when navigating to the setup page. - Refined project restoration logic to use merge for system data, preserving required structures.
- Introduced `.github/workflows/e2e-check.yml` to ensure E2E signature validation on pull requests to `main`. - Added `scripts/e2e-verify.sh` to automate E2E test execution and commit verification. - Updated `package.json` with `test:e2e:verify` script binding.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/logic/project-lifecycle.ts (1)
293-311:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse stashed
systemas the restoration baseline to prevent artifact carry-over.Current merge direction can keep transient learning-mode fields from the live project document. That risks restoring a “mostly old + some learning leftovers” system state.
♻️ Proposed fix
- // Restore system data via merge to prevent artifact survival while preserving required structures - if (projectDataFlags.stashedSystem) { - clonedData.system = FoundryUtils.mergeObject( - clonedData.system || {}, - projectDataFlags.stashedSystem as unknown as object, - ); - } + // Restore from stashed snapshot first to avoid carrying learning-only artifacts + if (projectDataFlags.stashedSystem) { + clonedData.system = FoundryUtils.deepClone( + projectDataFlags.stashedSystem as unknown as object, + ); + } clonedData.flags = { ...(clonedData.flags as object), ...completedFlags, }; // Ensure activities are restored to their stashed state (or empty) - if (clonedData.system) { - (clonedData.system as any).activities = FoundryUtils.deepClone( - projectDataFlags.stashedActivities || {}, - ); - } + if (!clonedData.system) clonedData.system = {}; + (clonedData.system as any).activities = FoundryUtils.deepClone( + projectDataFlags.stashedActivities || {}, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/logic/project-lifecycle.ts` around lines 293 - 311, The merge currently uses clonedData.system as the base and merges stashedSystem into it, which can preserve transient fields from the live document; change the merge so projectDataFlags.stashedSystem is the baseline and clonedData.system is merged into it (i.e., swap the arguments to FoundryUtils.mergeObject or otherwise assign clonedData.system = merge(stashedSystem, clonedData.system)), and keep the activities restoration using projectDataFlags.stashedActivities via FoundryUtils.deepClone to ensure activities come only from the stashed state; update references to projectDataFlags.stashedSystem, clonedData.system, FoundryUtils.mergeObject, projectDataFlags.stashedActivities, and FoundryUtils.deepClone 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 `@e2e/00-data-setup.spec.ts`:
- Around line 20-38: The compendium pack variable (pack) obtained from (game as
any).packs.get(`world.${c.name}`) may remain undefined after calling
foundry.documents.collections.CompendiumCollection.createCompendium, causing
subsequent featPack.getDocuments() to throw; after creating a compendium inside
the compendiums loop re-fetch the pack by calling (game as
any).packs.get(`world.${c.name}`) again (or throw an explicit error if still
undefined) so that later calls like featPack.getDocuments() operate on a valid
pack; ensure you also re-fetch the specific featPack (world.test-learning-feats)
before calling getDocuments() to avoid using a stale/undefined handle.
In `@e2e/multi-user.spec.ts`:
- Around line 33-37: playerPage.goto("/join") fails because a new context
created by playerContext = await browser.newContext(...) does not inherit the
test runner's baseURL; update the newContext call (playerContext) to include the
same baseURL used by the test runner or change the navigation to use an absolute
URL when calling playerPage.goto, ensuring you modify the
playerContext/newContext instantiation or playerPage.goto call accordingly.
In `@scripts/e2e-verify.sh`:
- Around line 7-12: The current clean-worktree check in scripts/e2e-verify.sh
only detects tracked changes; update the check to reject untracked files as well
by replacing the git diff-index call with a command that fails when git status
reports any changes (including untracked), e.g. use git status --porcelain and
test for non-empty output; modify the same block that currently uses git
diff-index --quiet HEAD -- so the script echoes the same error messages and
exits when git status --porcelain returns any lines.
---
Outside diff comments:
In `@src/logic/project-lifecycle.ts`:
- Around line 293-311: The merge currently uses clonedData.system as the base
and merges stashedSystem into it, which can preserve transient fields from the
live document; change the merge so projectDataFlags.stashedSystem is the
baseline and clonedData.system is merged into it (i.e., swap the arguments to
FoundryUtils.mergeObject or otherwise assign clonedData.system =
merge(stashedSystem, clonedData.system)), and keep the activities restoration
using projectDataFlags.stashedActivities via FoundryUtils.deepClone to ensure
activities come only from the stashed state; update references to
projectDataFlags.stashedSystem, clonedData.system, FoundryUtils.mergeObject,
projectDataFlags.stashedActivities, and FoundryUtils.deepClone 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ea1c0d1-4128-4517-b537-a94170ae62fa
📒 Files selected for processing (9)
.github/workflows/e2e-check.ymle2e/00-data-setup.spec.tse2e/helpers.tse2e/multi-user.spec.tse2e/time-bank.spec.tspackage.jsonplaywright.config.tsscripts/e2e-verify.shsrc/logic/project-lifecycle.ts
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/time-bank.spec.ts
…nsistency - Updated `scripts/e2e-verify.sh` to replace commit-based verification with file content hashing for enhanced accuracy. - Improved `.github/workflows/e2e-check.yml` to validate E2E verification using a consistent hash calculation method.
…cleanup test - Updated `mergeObject` mock to support `recursive`, `insertKeys`, and `overwrite` options for improved flexibility in target merging. - Added a test case to validate transient field cleanup during project recreation in `project-lifecycle.test.ts`. - Updated `project-lifecycle.ts` to adjust merge logic with `insertKeys: false` for stashed system restoration.
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 @.github/workflows/e2e-check.yml:
- Around line 7-10: The check-e2e job currently lacks explicit permissions; add
a job-level permissions block under the check-e2e job (jobs -> check-e2e) with
permissions: contents: read to enforce least-privilege for actions/checkout and
file verification; place this permissions key before steps so the job token is
limited to read-only repository contents.
- Around line 11-13: Replace the mutable tag `actions/checkout@v4` in the
workflow with a specific immutable commit SHA (e.g.,
`actions/checkout@<full-sha>`) and optionally keep a trailing comment noting the
corresponding tag (for example `# v4.x.y`) so future reviewers know the intended
release; this change should be made in the e2e-check.yml where `uses:
actions/checkout@v4` appears and preserve the existing `with: fetch-depth: 0`
block unchanged.
- Around line 24-27: The hash pipeline that currently runs "git ls-files ... |
sort | xargs sha256sum | sha256sum" is unsafe for filenames with
spaces/newlines; update the pipeline to use null delimiters by changing the "git
ls-files" invocation to include -z, change "sort" to use -z (or
--zero-terminated), and change "xargs" to use -0 so the sequence becomes git
ls-files -z ... | sort -z | xargs -0 sha256sum | sha256sum; apply the same
change to the identical pipeline found in the other script so both use -z/-0 for
filename-safe hashing.
In `@src/logic/project-lifecycle.ts`:
- Around line 295-299: The current merge uses the live item's system as the
target so mutated live fields win; instead restore stashed project-owned system
fields by making the stashed copy the base so it takes precedence. Replace the
FoundryUtils.mergeObject call so you deepClone(projectDataFlags.stashedSystem)
is the first argument and merge clonedData.system (or {}) into it (or simply
assign the deep-cloned stash) — e.g., use
FoundryUtils.deepClone(projectDataFlags.stashedSystem) as the base when calling
FoundryUtils.mergeObject or directly assign clonedData.system =
FoundryUtils.deepClone(projectDataFlags.stashedSystem) so stashed
system.description.value and system.type.value are restored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c28cfb98-2ee2-4389-b921-e0f770f7966e
📒 Files selected for processing (7)
.e2e-verification.github/workflows/e2e-check.ymle2e/00-data-setup.spec.tsscripts/e2e-verify.shsrc/logic/project-lifecycle.tstests/logic/project-lifecycle.test.tstests/setup.ts
✅ Files skipped from review due to trivial changes (1)
- .e2e-verification
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/e2e-verify.sh
- tests/setup.ts
- e2e/00-data-setup.spec.ts
- Improved hash calculation in `e2e-verify.sh` and `.github/workflows/e2e-check.yml` by enabling null-terminated file lists for better handling of special characters. - Simplified stashed system restoration in `project-lifecycle.ts` by removing unnecessary merge logic. - Adjusted corresponding test expectation in `project-lifecycle.test.ts` to reflect restored value from stashed data.
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/multi-user.spec.ts (1)
33-37: ⚡ Quick winClose
playerContextin afinallyblock.Right now, any failure before Line 179 skips cleanup and can leak a browser context in the worker.
Suggested patch
const playerContext = await browser.newContext({ baseURL, viewport: { width: 1920, height: 1080 }, }); // Fresh context with required resolution -const playerPage = await playerContext.newPage(); -await playerPage.goto("/join"); -... -await playerContext.close(); +try { + const playerPage = await playerContext.newPage(); + await playerPage.goto("/join"); + ... +} finally { + await playerContext.close(); +}Also applies to: 179-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/multi-user.spec.ts` around lines 33 - 37, Wrap the code that uses playerContext (and the other context opened around line ~179) in a try/finally so the context is always closed: after creating playerContext and playerPage, run your test actions in a try block and call await playerContext.close() in the finally block (do the same for the other context opened later). Reference playerContext and playerPage to locate where to add the try/finally and ensure await playerContext.close() (and the other context's close) is always executed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/multi-user.spec.ts`:
- Around line 59-72: The test must reset PC 3's projects before enabling
auto-spend to avoid stale "Second Project" interfering with single-project
assertions; inside the playerPage.evaluate callback that finds actor = (game as
any).actors.getName("PC 3") and before setting settings via (game as
any).settings.set(moduleId, ...), remove or trim actor's extra projects so only
one active project remains (e.g., update the actor's project list/embedded
documents to a single project) and persist that change via actor.update(...) or
the game's appropriate actor API, then proceed to set autoSpend and
autoSpendUnits.
---
Nitpick comments:
In `@e2e/multi-user.spec.ts`:
- Around line 33-37: Wrap the code that uses playerContext (and the other
context opened around line ~179) in a try/finally so the context is always
closed: after creating playerContext and playerPage, run your test actions in a
try block and call await playerContext.close() in the finally block (do the same
for the other context opened later). Reference playerContext and playerPage to
locate where to add the try/finally and ensure await playerContext.close() (and
the other context's close) is always executed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed578606-7586-4365-a548-f136cdf2f0cd
📒 Files selected for processing (8)
.e2e-verification.github/workflows/e2e-check.ymle2e/00-data-setup.spec.tse2e/multi-user.spec.tsscripts/e2e-verify.shsrc/logic/project-lifecycle.tstests/logic/project-lifecycle.test.tstests/setup.ts
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/e2e-check.yml
- tests/setup.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .e2e-verification
- tests/logic/project-lifecycle.test.ts
- scripts/e2e-verify.sh
- Added logic to remove redundant learning projects from actor items in `multi-user.spec.ts` to prevent interference during tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores