Skip to content

refactor: migrate units/states/tasks/tasks.coffee from CoffeeScript to Angular#493

Open
Sujay-Deakin wants to merge 7 commits into
thoth-tech:10.0.xfrom
Sujay-Deakin:migrate/units-states-tasks-coffee
Open

refactor: migrate units/states/tasks/tasks.coffee from CoffeeScript to Angular#493
Sujay-Deakin wants to merge 7 commits into
thoth-tech:10.0.xfrom
Sujay-Deakin:migrate/units-states-tasks-coffee

Conversation

@Sujay-Deakin
Copy link
Copy Markdown

Description

Migrates units/states/tasks/tasks.coffee from CoffeeScript/AngularJS to TypeScript/Angular
as part of the ongoing Angular migration effort.

Changes

  • Created tasks.component.ts with UnitsTasksStateComponent replacing UnitsTasksStateCtrl
  • Created tasks.component.html with <ui-view> as the router outlet
  • Created tasks.component.scss (no custom styles needed)
  • Registered UnitsTasksStateComponent in doubtfire-angular.module.ts
  • Downgraded component in doubtfire-angularjs.module.ts for hybrid compatibility
  • Removed old import 'build/src/app/units/states/tasks/tasks.js' from doubtfire-angularjs.module.ts
  • Deleted old tasks.coffee

Bug Fixes Applied During Migration

  • Null-safe parsing of taskKey from URL params
  • Force taskKeyString to a string before passing to newTaskService
  • Avoid redundant $state.go when task is null
  • Cleaner taskKey assignment using optional chaining
  • Safer preventDefault condition with state name null checks

Testing

  • Component registers correctly in the Angular module
  • Downgraded directive available to AngularJS hybrid app
  • taskData object initialises with correct default values
  • taskKey syncs correctly from URL params on load

@Thirus224849242
Copy link
Copy Markdown

The migration appears incomplete. The last commit restores the full AngularJS state config and UnitsTasksStateCtrl controller inline in doubtfire-angularjs.module.ts, which means routing is still handled by AngularJS and the new Angular component is never actually used. The tasks.coffee logic has been relocated rather than migrated.

There is also a behaviour regression in setTaskKeyFromUrlParams. Falling back to an empty string when taskKeyString is null means taskKeyFromString is always called on init, potentially setting taskKey to an unexpected value instead of null. The original only called this function when taskKey was truthy, so a null guard should be added before calling the service.

It would also be worth confirming that TaskService from src/app/api/services/task.service actually exposes taskKeyFromString. The Angular component uses TaskService while the AngularJS controller uses newTaskService, and these may not be the same. (I might be wrong).

Finally, the transitionService.onStart hook uses empty criteria, so it fires on every state transition in the app rather than just those relevant to this state. It should be scoped to units/tasks to avoid unintended side effects.

The defensive null checks and removal of the old import are good. The routing side may need changes.

@Sujay-Deakin
Copy link
Copy Markdown
Author

Hi Thirumal, thanks for the detailed feedback! I've addressed all four points in the latest commit:

  1. Routing now uses the Angular component; removed the UnitsTasksStateCtrl controller entirely. The state config now uses the template '', which loads the Angular component directly, so routing is handled entirely by Angular.

  2. Null guard added to setTaskKeyFromUrlParams; removed the ?? '' fallback. The service is now only called when taskKeyString is truthy, matching the original behaviour.

  3. TaskService vs newTaskService; confirmed they are the same service. newTaskService in AngularJS is just a downgraded version of TaskService, as seen in doubtfire-angularjs.module.ts

  4. Scoped the transitionService.onStart hook; changed from {} to {to: 'units/tasks.**'} so it only fires for transitions into units/tasks child states.

Tested locally, inbox and task explorer both load correctly after these changes. Let me know if anything else needs addressing!

Copy link
Copy Markdown

@sakethsram8888 sakethsram8888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed and tested locally
looks like there are issues with the code and the app accessing the browser, it doesnt load

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants